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

Updated 'Hack' to only overwrite final prediction mvals instead of all mvals + PEP8 #121

Merged
merged 1 commit into from
May 16, 2022

Conversation

jskrist
Copy link
Member

@jskrist jskrist commented May 13, 2022

Description

There was a "Hack" in the model.py code to make sure that calculated data was returned for all entries of the mvals. This code essentially copies the second-to-last value (which is computed by the calc function) into the last value as well. This logic works fine for predicted values, and results in the last two data points being the same exact value. However, for non-predicted values, this results in overwritten input data. Most of the time, the input states aren't changing significantly in the last two data point, so this doesn't cause any issues. However, when the last data point of an input state does change significantly, then the returned mvals will no longer match the given inputs.

The fix presented in this PR is to only overwrite the last values for components that are predictions, and to leave non-predicted values alone.

Interface impacts

This does not change the API, and should have minimal impacts on the user community in most cases.

Testing

Unit tests

I was unable to run the unit test on Windows due to not having the Ska data available locally.

  • No unit tests

Independent check of unit tests by @taldcroft

  • Mac

Functional tests

This was tested using the FOT MATLAB Tools running MCC and evaluating all the thermal models for a given week. The thermal predictions were checked against the values without this fix, and the mvals from non-predicted components were compared to input states.

In particular, the final value of the fep_count component of the ACIS DPA model was compared to the provided inputs from the MAY0222A schedule.

@jskrist jskrist requested a review from jzuhone May 13, 2022 18:05
Comment on lines +690 to +691
# hackish fix to ensure last value is a computed value for predicted components
self.mvals[:self.n_preds, -1] = self.mvals[:self.n_preds, -2]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the meat of the PR. The rest is just PEP8 improvements.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good.

@taldcroft taldcroft changed the title fixed some PEP8 violations and updated 'Hack' to only overwrite final… Updated 'Hack' to only overwrite final prediction mvals instead of all mvals + PEP8 May 16, 2022
@taldcroft
Copy link
Member

Thanks @jskrist !

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