search

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

Does `System` still have `_TEA` attribute? #64

Closed yalinli2 closed 3 years ago

yalinli2 commented 3 years ago

I use _TEA in some of my code and looks like it's gone now (would still like to have it if there's no strong reason to remove it)? Still seeing some legacy in BioSTEAM, though, thanks!

https://github.com/BioSTEAMDevelopmentGroup/biosteam/blob/68141e67703800cfc0bc326cf5762ae87316259d/biosteam/_tea.py#L341

yoelcortes commented 3 years ago

Hi Yalin,

I can add it back, I removed it because it didn't serve a purpose in BioSTEAM but I didn't realize it was being used in QSDsan.

Thanks,

yoelcortes commented 3 years ago

@yalinli2, could you add QSDsan tests to BioSTEAM? That way we can avoid issues like these. You'll need to expose the unit tests in QSDsan's installation. You can also checkout the "biosteam\tests\test_biorefineries.py" for an example on how to import tests from another library.

Alternatively, we can add a test in BioSTEAM each time we have a dependency issue.

yalinli2 commented 3 years ago

This sounds great! I'll add QSDsan's tests after I release a new version in the next couple of days (or the tests will certainly fail because of this). I'll make a new branch with this minor change and close the issue then.

About _TEA, you can decide whether you want it or not, I just feel sometimes it's convenient if you need to retrieve the TEA object for multiple systems, I'm using a dict now instead, thanks!

yoelcortes commented 3 years ago

Cool, I'm happy to make modifications and keep features (even private methods and attributes) if it means downstream dependencies can have an easier time. Since you've already implemented an alternative to the <System>._TEA attribute, I'll opt for not adding back the attribute. Thanks :)

yalinli2 commented 3 years ago

Well, can we have the TEA or _TEA attribute back? I think at least we are using it in making the system report: https://github.com/BioSTEAMDevelopmentGroup/biosteam/blob/8d207fe778f8093c31660e24db409ad2c3770ebc/biosteam/report/table.py#L97

Thanks!

yoelcortes commented 3 years ago

Ahhh, sure thing, let's just keep it... maybe others may also be using the _TEA/TEA attributes in biorefineries.

Edit: I'll also add tests for the system report with TEA.

Thanks!

yalinli2 commented 3 years ago

Thanks Yoel, this is great! I just made a new branch with test for qsdsan, also add an LCA attribute to make it more convenient to retrieve LCA objects (unfortunately I hadn't started on making this... the QSDsan office hour/tutorial takes up all my spare time...).

There's also another minor bug in _state.py, where there is no _skip attribute, so I commented out that line: https://github.com/BioSTEAMDevelopmentGroup/biosteam/blob/a51f5b9dd73acc47ea9217bfa0f642327e08b2f3/biosteam/evaluation/_state.py#L122

Those were all my changes, but unfortunately I let Spyder automatically remove the trailing spaces and extra blank lines, so there seemed to be a ton of changes (including in table.py where I was debugging... I didn't change anything there, I should stop using git add .). Sorry about this! You can change as you see fit, thanks!