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

docstrings and instances added #56

Merged
merged 4 commits into from
Dec 17, 2024

Conversation

gpap-gpap
Copy link
Contributor

@gpap-gpap gpap-gpap commented Aug 18, 2024

@berland pinging you because I am not sure an automated email gets sent with draft PRs

Feel I should contribute to this as I was using it today to test some code I was writing internally. I made four changes:

  • Used type hints at base level of methods (i.e. not in submethods), and declarations involiving Material class. (this makes use of future import for old versions which may be a bummer for the devs so happy to roll back)
  • Rewrote the docstrings for the methods adding detail wrt to previous docstring and some accuracy as to what is being returned (instance of a class with bound moduli rather than a "gives a bound on moduli") as well as examples. If there is a style guide that new docstrings violate happy to roll back
  • generate class instance and only return it if class instantiated successfully. Not sure this can ever fail the way things are written now, but I don't see how it can hurt. Again, if this contradicts with internal style guide happy to roll back.
  • I also wrote a check so that hashin_shtrikman_walpole bound argument is checked to be either of lower and upper (it was written so that any string other than lower would give the upper bound. This could be breaking if someone somewhere has misspelled code!)

I have identified some inconsistency regarding the types of attributes in the Material class, namely generating a material with hashin_shtrikman_average has float attributes which I feel is discouraged over array of a single float. But this doesn't seem enforced so I haven't done anything about that.

@eivindjahren eivindjahren added the christmas-review Issues and PRs for Christmas review label Dec 13, 2024
@eivindjahren eivindjahren self-requested a review December 17, 2024 12:42
@eivindjahren eivindjahren removed the christmas-review Issues and PRs for Christmas review label Dec 17, 2024
@eivindjahren
Copy link
Collaborator

Sorry for taking so long to get to this, this unfortunately got lost in our notifications. I will get on this as soon as I can.

@eivindjahren eivindjahren marked this pull request as ready for review December 17, 2024 12:57
@eivindjahren
Copy link
Collaborator

eivindjahren commented Dec 17, 2024

I rolled back the reraise of ValueError as you mentioned, I agree that this handling is not great, but I would need to rethink it in more detail.

The inconsistency is just that float and np.vector[float] are used interchangeably throughout. Again, something to make more consistent.

@eivindjahren eivindjahren merged commit 52a9664 into equinor:main Dec 17, 2024
5 of 6 checks passed
@eivindjahren
Copy link
Collaborator

Thank you so much for your contribution, sorry for taking so long to get to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants