Skip to content

Conversation

@teubert
Copy link
Contributor

@teubert teubert commented Jul 13, 2023

No description provided.

@teubert teubert requested a review from mrzkp July 13, 2023 18:33
@github-actions
Copy link

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.

@mrzkp
Copy link
Contributor

mrzkp commented Jul 13, 2023

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

params['units'] = [params['units'] for _ in range(params['layers'])]
if not isinstance(params['units'], (list, np.ndarray)):
if not isinstance(params['units'], (list, np.ndarray, tuple)):
raise TypeError(f"units must be a list of integers, not {type(params['units'])}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the exceptions to mention that 'units must be a sequence of integers' instead of 'a list'?

@teubert
Copy link
Contributor Author

teubert commented Jul 13, 2023

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.

@mrzkp
Copy link
Contributor

mrzkp commented Jul 13, 2023

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
Copy link

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%) ⬆️

@teubert teubert merged commit 1485c8a into dev Jul 13, 2023
@teubert teubert deleted the feature/tuple_units branch July 18, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants