watertap-org / watertap

The WaterTAP development repository
https://watertap.readthedocs.io/en/latest
Other
59 stars 57 forks source link

Address numerical stability issues associated with DSPM-DE property package #916

Closed bknueven closed 1 year ago

bknueven commented 1 year ago

We've seen occasional issues with unit models using the DSPM-DE model (see #860, https://github.com/watertap-org/watertap/actions/runs/3650225701/jobs/6165944727#step:11:5728, https://github.com/watertap-org/watertap/issues/695#issuecomment-1415978648).

There is likely an issue with either scaling in the DSPM-DE property package, the unit models which utilize it, or both. There could be some other subtle issue (see AMPL evaluation errors from https://github.com/watertap-org/watertap/actions/runs/3650225701/jobs/6165944727#step:11:5728).

adam-a-a commented 1 year ago

I looked at those failing tests and tried troubleshooting locally (a while ago so memory rusty). It was difficult to truly know what changed between the two instances of the model as a result of the changes to the "MCAS" property model in draft PR #860. If there are issues with scaling or something else with NF DSPMDE, I didn't see how the changes in #860 could lead to or expose such issues.

I think there are tools to address this but haven't taken the time and effort to dig deeper. @bknueven are you aware of Pyomo/IDAES tools available for copying a snapshot of your model and exporting it? Beyond that, if we could take snapshots of the NF DSPMDE model as it exists with the MCAS implementation on main and compare it directly to a snapshot of the NF DSPMDE model as a result of the property package mod on #860, it would be far easier to see what the subtle differences are between models and work towards a solution.

I'm pretty sure there are tools in place to copy/export models to json or something like that, but not as sure about whether we have anything that can cross-check two models and highlight the differences (that would be cool and useful). I don't want any of us reinventing the wheel if such tools do exist somewhere.

lbibl commented 1 year ago

I was able to locate (if accurately enough) what in the previous PR #860 failed (inconsistently over different platforms) being its different domain of the ion_set. The previous PR 860 defined ion_set by taking the union of cation and ion so made its domain NOT being any like the original. Modifying this to be any now let PR#860 pass all the tests and should good to merge upon final reviews.

Nonetheless, I think this issue remains since the previous failure did suggest more accurate indexing of some properties in nf_dspmde could be needed. I could see in the model a few properties that should only carried by ions were indexed or looped in all solutes (previously ion_set | solute_set and now solute_set). Although this hasn't caused errors thus far (the ion_set domain modification is kinda postponing the revelation of this error otherwise wouldn't have been needed since the narrower domain is accurate ) , it would when dealing with a solution actually having both ion and neutral species. Overall, I recommend, later on, the refinement of nanofiltration_DSPMDE in details to amend all inaccurate indices.

adam-a-a commented 1 year ago

Although this hasn't caused errors thus far (the ion_set domain modification is kinda postponing the revelation of this error otherwise wouldn't have been needed since the narrower domain is accurate ) , it would when dealing with a solution actually having both ion and neutral species.

This was one of the reasons that I was originally resistant to making this change with the sets, but I am glad you resolved the issue with PR #860 !

adam-a-a commented 1 year ago

@lbibl @bknueven do we think that this issue can be closed now?

bknueven commented 1 year ago

@lbibl @bknueven do we think that this issue can be closed now?

I think this can be closed. I don't think the issue is with the property package per se, but rather handling scaling correctly in our unit models when components get filtered out.

lbibl commented 1 year ago

I am ok with closing.