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

Expose model configuration options #1085

Closed
wants to merge 2 commits into from

Conversation

dangunter
Copy link
Collaborator

@dangunter dangunter commented Jul 16, 2023

Fixes #1084

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Attention: Patch coverage is 90.56604% with 5 lines in your changes missing coverage. Please review.

Project coverage is 95.52%. Comparing base (d81564b) to head (a8db69f).
Report is 270 commits behind head on main.

Files Patch % Lines
watertap/ui/fsapi.py 90.56% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
+ Coverage   95.37%   95.52%   +0.14%     
==========================================
  Files         315      318       +3     
  Lines       30950    31049      +99     
==========================================
+ Hits        29520    29659     +139     
+ Misses       1430     1390      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Jul 20, 2023
@ksbeattie
Copy link
Contributor

@dangunter, we put this (and the chain of 2 issues) on the Sept release board on the assumption that this will be ready in Sept.

@dangunter dangunter marked this pull request as ready for review July 22, 2023 00:18
@dangunter
Copy link
Collaborator Author

@MichaelPesce marking ready for review to simplify any contributions/testing. As mentioned in watertap-org/watertap-ui#90 , the main to-do here is to show how the option value(s) are used in building/solving e.g. the nanofiltration flowsheet, and documenting how to use when wrapping new models.

@adam-a-a
Copy link
Contributor

@dangunter - one thought about this functionality- selecting a specific configuration option could change whether certain variables should be available for the user to touch. One way to deal with this would be to be able to specify behind the scenese which vars would be "grayed out" and inaccessible with particular config options. It'd probably make sense to test this out starting simple with just an RO unit alone. Alternatively, we could look at this from the perspective of a given flowsheet, where you might want to toggle between predetermined variations of a given flowsheet. Personally, I think the former example would be better to start with.

@adam-a-a
Copy link
Contributor

@dangunter @MichaelPesce I am trying to get back up to speed with state of the UI. I am behind at this point.

Is this PR still relevant, or is this one already covered by other UI-related PRs? Please let me know so we can decide whether to review or close this one.

@adam-a-a
Copy link
Contributor

This PR was superseded by #1152

@adam-a-a adam-a-a closed this Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable configuration of model options in UI
3 participants