Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set Model parameter distributions and load statements using DataFrame or spreadhseet #209

Merged
merged 9 commits into from
Nov 1, 2024

Conversation

sarangbhagwat
Copy link
Member

Adds a new EasyInputModel class that lets users set parameter distributions and load statements using DataFrame objects or spreadsheet files.

…ents configurable through dataframes or spreadsheets
@yoelcortes
Copy link
Member

yoelcortes commented Oct 31, 2024

@sarangbhagwat,

This looks like a fun way to create models. I hope we can work it into the main code a little more. I would suggest the following edits:

  1. Remove the subclass, add the new method to the Model class, rename the new method to parameters_from_df.
  2. Remove namespace_dict from the model __init__, leave it as an input to the parameters_from_df method and rename it to namespace (considering python namespaces are dictionaries, it think we can opt for a shorter name).
  3. Replace namespace default to None (using mutable objects may lead to issues later).
  4. Add new feature to autofill the namespace with all objects in the flowsheet (except if name is already taken):
if namespace is None: 
    namespace = self.system.flowsheet.to_dict()
else:
    namespace = self.system.flowsheet.to_dict() | namespace
  1. Add a test in tests/test_evaluation.py

Thank you! Let me know your thoughts or if you'd like me to help with any implementation.

@sarangbhagwat
Copy link
Member Author

Yeah! Pretty much all the recent BioSTEAM biorefineries (TAL, HP, succinic) are using this to load parameter distributions, it's really useful close to publication when you can essentially just copy the table from your spreadsheet and paste it into your supporting information, haha. I'd implemented this feature in BioIndustrial-Park, just moving it to BioSTEAM.

Thanks, all great suggestions -- I've changed the code for the namespace dict merge slightly, let me know if you would prefer it the other way. Also added tests -- I've tried to cover most of the new features.

@sarangbhagwat sarangbhagwat requested review from yoelcortes and yalinli2 and removed request for yoelcortes October 31, 2024 22:38
Copy link
Member

@yoelcortes yoelcortes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, now we have a clean way to define parameters in a spreadsheet. I appreciate you taking the time to move this feature from your biorefineries to core biosteam and addressing everything in the review.

By the way, I removed the left over EasyInputModel subclass since its main feature was already added to the Model class.

If you'd like to add aliases for backwards compatibility (avoiding duplicate code); I would be OK with that. For example, adding the following lines is OK.

EasyInputModel = Model
Model.load_parameter_distributions = Model.parameters_from_df

Feel free to merge whenever,
Thank you!

@sarangbhagwat sarangbhagwat merged commit e2d3942 into master Nov 1, 2024
1 check failed
@sarangbhagwat sarangbhagwat deleted the easy_input_model_stage branch November 1, 2024 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants