Allow units to be specified as tuple

nasa / progpy

The NASA Prognostic Python Packages is a Python framework focused on defining and building models and algorit for prognostics (computation of remaining useful life) of engineering systems, and provides a set of models and algorithms for select components developed within this framework, suitable for use in prognostic applications.

https://nasa.github.io/progpy/

Other

55 stars 8 forks source link

Allow units to be specified as tuple #66

Closed teubert closed 1 year ago

github-actions[bot] commented 1 year ago

Thank you for opening this PR. Each PR into dev requires a code review. For the code review, look at the following:

[ ] Reviewer (someone other than author) should look for bugs, efficiency, readability, testing, and coverage in examples (if relevant).
[ ] Ensure that each PR adding a new feature should include a test verifying that feature.
[ ] All errors from static analysis must be resolved.
[ ] Review the test coverage reports (if there is a change) - will be added as comment on PR if there is a change
[ ] Review the software benchmarking results (if there is a change) - will be added as comment on PR
[ ] Any added dependencies are included in requirements.txt, setup.py, and dev_guide.rst (this document)
[ ] All warnings from static analysis must be reviewed and resolved - if deemed appropriate.

aqitya commented 1 year ago

We should also change the documentation of what units can be on line 428 of the lstm_model.py file.

teubert commented 1 year ago

We should also change the documentation of what units can be on line 428 of the lstm_model.py file.

Actually, I disagree on this one. Any way we change it I think it would make this more confusing for the user. I think it's fine to just have it as an undocumented feature and list one sequence type in the documentation.

aqitya commented 1 year ago

We should also change the documentation of what units can be on line 428 of the lstm_model.py file.

Actually, I disagree on this one. Any way we change it I think it would make this more confusing for the user. I think it's fine to just have it as an undocumented feature and list one sequence type in the documentation.

Makes sense. I was going to offer a suggested change, but it was a bit hard to come up with one. Leaving it as it is works!

codecov-commenter commented 1 year ago

Codecov Report

Merging #66 (d650984) into dev (aaa4a6b) will increase coverage by 0.00%. The diff coverage is 25.00%.

@@           Coverage Diff           @@
##              dev      #66   +/-   ##
=======================================
  Coverage   84.19%   84.19%           
=======================================
  Files          99       99           
  Lines       10110    10111    +1     
=======================================
+ Hits         8512     8513    +1     
  Misses       1598     1598

Impacted Files	Coverage Δ
src/progpy/data_models/lstm_model.py	`7.21% <25.00%> (+0.24%)`	:arrow_up: