BioSTEAMDevelopmentGroup / biosteam

The Biorefinery Simulation and Techno-Economic Analysis Modules; Life Cycle Assessment; Chemical Process Simulation Under Uncertainty
Other
175 stars 35 forks source link

`Unit._check_run` may cause problem for auxiliary units #137

Closed yalinli2 closed 1 year ago

yalinli2 commented 1 year ago

@yoelcortes I think this was probably added to make sure that the users are not manually adding the costs/utilities of auxiliary units since they are automatically added now https://github.com/BioSTEAMDevelopmentGroup/biosteam/blob/cd172aaba8cb747ca6e7c7a5367b8242925ed2e0/biosteam/_unit.py#L923

But some recent changes are causing problems for the heat exchangers that I added as auxiliary units for another unit. If you run the entire script of comparison.py of the wwt module then test_cs_baseline(), you should see the error. Thanks!

yalinli2 commented 1 year ago

Also, I'm wondering if this check really makes sense (i.e., should we forbid users from having run in design/cost and vise versa)?

For example (a separate scenario from the error above), what if I have a unit which has an auxiliary HX whose duty depends on the heat loss, and the heat loss depends on the surface of this unit which is calculated in _design, so I'll need to call the auxiliary HX's simulate_as_auxiliary_exchanger in _design and I think it'd also trigger this error.

yoelcortes commented 1 year ago

@yalinli2, thanks for posting! I know code changes are quite a hassle, especially for big projects. The recent changes are quite big, but makes it possible streamline auxiliaries better. The issue you were running into had to do with the following:

  1. The heat exchanger heat utility was added in the __init__ method. Just remove this line self.heat_utilities = hx.heat_utilities from your inherited units.
  2. Auxiliary pumps were registered in the flowsheet (e.g., bst.Pump(f'{self.ID}_eff')). Although auxiliaries should not be in the system path, the auxiliary pump was included because it was in the flowsheet. When BioSTEAM goes to compile results for the pump in the system path, it was already compiled in the owner unit operation. I just made changes to BioSTEAM so that auxiliary units are not included in the system path anymore (76025a8d55dced18d09f0ad644446ec47e2a96a5). You should not have to worry about this issue anymore, but now you can also avoid registering units by adding a leading dot (".") to the ID. For example:
from biosteam import settings, Mixer, F
settings.set_thermo(['Water'])
M1 = Mixer('.M1')
assert M1.ID == 'M1'
assert M1 not in F.unit

Auxiliary units are not compiled until after _design and _cost, so you can feel free to simulate auxiliaries in _run, _design, and _cost (doesn't matter). The _check_run method makes sure that utility and cost results are not added to the main unit operation in _run, which can cause issues in recycle loops if previous results are not cleared.

I can take care of updating the wwt branch with comments regarding biosteam updates if you give me the green light. Just let me know!

Thanks!

yalinli2 commented 1 year ago

Hi @yoelcortes , thanks for looking into this, but can you let me know where this line self.heat_utilities = hx.heat_utilities was? I thought I removed this after I pulled in the recent changes related to HX

The heat exchanger heat utility was added in the init method. Just remove this line self.heat_utilities = hx.heat_utilities from your inherited units.

I added the dot to the auxiliary units and it seems to be working for the cornstover biorefinery, but the results changed and I'm not sure whether the before-change results were correct or the current ones are, I also haven't checked the other biorefineries yet. So I might come back on this.

For the wwt branch, please do not make changes now, I'll send in a draft PR when I feel comfortable, but I don't want to start making a bunch of changes while still haven't made sure that the auxiliary units are added properly.

I also want to avoid any major changes to the biorefineries before I submit the WWT design manuscript. Every time there are changes in the configuration (the most recent example is the auto populate function for the WWT module), I have to spend a few days checking the stream connections and see if the results are consistent (they often aren't), then I'll have to update every number in the manuscript which is very frustrating.

And updating the WWT module for the biorefineries at this stage will make it impossible (or at least make it hard) for me to compare the results with the original WWT design, so I really want to avoid doing that, thanks!

yoelcortes commented 1 year ago

@yalinli2, OK! Sounds good, self.heat_utilities = hx.heat_utilities is in the master branch in https://github.com/BioSTEAMDevelopmentGroup/Bioindustrial-Park/blob/master/biorefineries/wwt/_internal_circulation_rx.py

By the way, there was a bug in the dot feature, which I did not realize/fix until today morning. Now it is fixed and has a test.

THanks,

yalinli2 commented 1 year ago

Ah I see, the wwt module in bioindustrial-park's master branch was outdated, when I posted the issue, I had updated them but still had the bug, sorry I forgot to mention that!

However your other fixes after I posted this issue fixed things. I'll close this for now and reopen if I have further problems, thanks @yoelcortes !