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

feat: add compressor inlet- and outlet pressures to models/train level #152

Merged
merged 15 commits into from
Sep 13, 2023

Conversation

frodehk
Copy link
Contributor

@frodehk frodehk commented Sep 5, 2023

Why is this pull request needed?

Add compressor inlet- and outlet pressures from input data to models/train level (as requested inlet- and outlet pressures). It is initially implemented at component level, this is now removed - as it is most relevant for the users at train-level. When implemented in web, the input pressures can be plotted together with calculated pressures. Make it easier to understand that in case of pressure control mechanisms are active, the requested (input) inlet and outlet pressures may differ from what is actually calculated by eCalc.

What does this pull request change?

  • graph_result.py: update and add new function to add pressures at compressor train-level
  • graph_result.py: remove pressures at compressor component level
  • Write the input pressures to dto.result.results.CompressorModelResult object
  • Update snapshots

Issues related to this change:

https://equinor-ecalc.atlassian.net/browse/ECALC-121
https://equinor-ecalc.atlassian.net/browse/ECALC-92

@frodehk frodehk requested a review from a team as a code owner September 5, 2023 11:42
@frodehk frodehk self-assigned this Sep 5, 2023
@TeeeJay
Copy link
Collaborator

TeeeJay commented Sep 5, 2023

We should have consumer systems that return sth else than null for requested pressures. Now all snapshot tests only return null meaning that we dont have any snapshot tests that has a train that returns data? Sounds weird. If that is the case, we should have at least one happy path case for that, ideally for each "possibility"/edge case. For the cases where it is not relevant, which is probably coverd in snapshot tests, it looks ok (null as in no data).

@frodehk
Copy link
Contributor Author

frodehk commented Sep 5, 2023

There is also a mismatch with the train/model timevector and the individual variables (e.g. rates, pressure etc) per now, i.e. in the way it is implemented in this PR. Think there is a need to slice the variables map as input to the temporal expression, to match correct time intervals.

@frodehk
Copy link
Contributor Author

frodehk commented Sep 6, 2023

We should have consumer systems that return sth else than null for requested pressures. Now all snapshot tests only return null meaning that we dont have any snapshot tests that has a train that returns data? Sounds weird. If that is the case, we should have at least one happy path case for that, ideally for each "possibility"/edge case. For the cases where it is not relevant, which is probably coverd in snapshot tests, it looks ok (null as in no data).

Seems like the csv-tests are the only ones using the advanced yaml (with more advanced settings for compressors and compressor systems) - but data for models/trains are only dumped to json. I think we should extend the json-tests with one using the advanced yaml-file.

@TeeeJay
Copy link
Collaborator

TeeeJay commented Sep 6, 2023

We should have consumer systems that return sth else than null for requested pressures. Now all snapshot tests only return null meaning that we dont have any snapshot tests that has a train that returns data? Sounds weird. If that is the case, we should have at least one happy path case for that, ideally for each "possibility"/edge case. For the cases where it is not relevant, which is probably coverd in snapshot tests, it looks ok (null as in no data).

Seems like the csv-tests are the only ones using the advanced yaml (with more advanced settings for compressors and compressor systems) - but data for models/trains are only dumped to json. I think we should extend the json-tests with one using the advanced yaml-file.

yes, or add a relevant test (can be really simple) to the existing json-export-tests

@frodehk frodehk force-pushed the add-compressor-pressures-to-model-results branch from 96e310b to aada5bb Compare September 6, 2023 11:06
@frodehk frodehk force-pushed the add-compressor-pressures-to-model-results branch 3 times, most recently from 0f36db6 to c60d50b Compare September 11, 2023 09:25
Copy link
Collaborator

@TeeeJay TeeeJay left a comment

Choose a reason for hiding this comment

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

all checks are ok, not sure what you meant by erroneous snapshot? did you perhaps just change to the values we got in CICD , but that it is still failing localyl for you?

@frodehk
Copy link
Contributor Author

frodehk commented Sep 13, 2023

all checks are ok, not sure what you meant by erroneous snapshot? did you perhaps just change to the values we got in CICD , but that it is still failing localyl for you?

All tests are ok now, as the snapshot assert is removed, i.e. not using snapshot. Originally it was using snapshot. The snapshot is generated by copying the output json from the test, to the snapshot directory. Then running the test with --snapshot-update. Then, locally all is fine but it fails in the pipeline (in the 10th decimal I think). But as you see, snapshot is not used now.

@frodehk frodehk force-pushed the add-compressor-pressures-to-model-results branch from fe15926 to 9735059 Compare September 13, 2023 12:24
@frodehk frodehk merged commit 9b95ee5 into main Sep 13, 2023
6 checks passed
@frodehk frodehk deleted the add-compressor-pressures-to-model-results branch September 13, 2023 13:00
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