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

DOC: Update list of functions that lose units #10848

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

rpsfonseca
Copy link
Contributor

Description

This pull request is to address the missing functions on the list that details which operations lead to Quantities losing units.

Fixes #10845

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

@rpsfonseca - thanks! But these should not just be added, as the text just above notes the list is for "prior to astropy 4.0 and numpy 1.17". So, there should instead be a new header entry (maybe below this one is best), along the lines of "Numpy array creation functions cannot be used to initialize Quantity". For that, I'd include the explicit example with np.full that doesn't work (as it is the one most likely to surprise anyone)

@astrofrog astrofrog modified the milestones: v4.0.3, v4.0.4 Oct 14, 2020
@rpsfonseca
Copy link
Contributor Author

@mhvk So you say to add a new section like this one with the title being the one you mentioned?

And the explicit example you also mention, is the one given by the author of #10836 ?

@mhvk
Copy link
Contributor

mhvk commented Oct 14, 2020

@rpsfonseca - Yes to both!

@rpsfonseca rpsfonseca force-pushed the update-docs-on-funcs-lose-units branch from dc84fa5 to e039882 Compare October 15, 2020 17:36
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now!

@mhvk mhvk merged commit 8094b7c into astropy:master Oct 15, 2020
@pllim pllim added the hacktoberfest-accepted PR that counts towards Hacktoberfest points for the author label Oct 15, 2020
@pllim
Copy link
Member

pllim commented Oct 15, 2020

Since the issue that this PR fixed was labeled as Hacktoberfest, I applied "hacktoberfest-accepted" label here. I am still not fully understanding how the new fest rules work... cc @astrojuanlu

Thank you, all!

@astrojuanlu
Copy link
Member

thanks @pllim ! I was about to do it myself 😊

@pllim
Copy link
Member

pllim commented Oct 15, 2020

For future reference, please do not skip CI when there is real code in the docs. This caused real failures, which I am patching it over at #10874 . FYI.

@rpsfonseca
Copy link
Contributor Author

Thank you for the help!
And sorry about skipping CI, I checked the CONTRIBUTING guide, and it mentioned that on documentation we could skip CI. But I will keep that in mind for future contributions.

@mhvk
Copy link
Contributor

mhvk commented Oct 16, 2020

@rpsfonseca - no worries, when I reviewed I noticed the few checks but that readthedocs was done, and I also thought that was OK. Having been around, I should have known better...

Question: would you be willing to adjust the CONTRIBUTING guide, adding a line about not skipping if there are doctests? (And for that one we can actually skip CI!)

@pllim
Copy link
Member

pllim commented Oct 16, 2020

Perhaps this line in CONTRIBUTING.md confusing: If your commit makes substantial changes to the documentation but no code changes (... the key is "no code changes" but changing code in doc is basically code changes because we test it.)

I also added something in https://docs.astropy.org/en/latest/development/docguide.html#adding-a-git-commit .

If any of those could be less confusing, please feel free to propose changes as PR. Thanks!

@rpsfonseca
Copy link
Contributor Author

@mhvk I'd love to help on that for sure!
@pllim Yes, that was the line the lead me into the "mistake".

I will look into that and propose a change and check your opinion.

eteq pushed a commit to eteq/astropy that referenced this pull request Oct 19, 2020
…-lose-units

DOC: Update list of functions that lose units
eteq pushed a commit that referenced this pull request Oct 19, 2020
…nits

DOC: Update list of functions that lose units
@pllim
Copy link
Member

pllim commented Oct 19, 2020

@mhvk , @eteq and I just found out that the UnitConversionError doesn't happen in Numpy 1.16.x, as with in Numpy 1.20.x. Is this expected? We dropped Numpy 1.16 on master but still tests against it on older branches.

>>> import numpy as np
>>> import astropy
>>> from astropy import units as u
>>> np.__version__
'1.16.5'
>>> astropy.__version__
'4.0.3'
>>> x = 1. * u.m
>>> np.full(10, x)
array([1., 1., 1., 1., 1., 1., 1., 1., 1., 1.])

pllim added a commit to pllim/astropy that referenced this pull request Oct 19, 2020
@mhvk
Copy link
Contributor

mhvk commented Oct 19, 2020

@pllim - I think 1.16 just completely ignored the unit, without even realizing it.
But I guess that means the text of the docs is still not quite right...

@pllim
Copy link
Member

pllim commented Oct 20, 2020

that means the text of the docs is still not quite right

We dropped numpy 1.16 in #10664 , so it is non-issue for master but for the sake of making the release out, Erik and I decided to just skip that doctest in 4.0.x and 4.1.x branches.

bsipocz pushed a commit that referenced this pull request Nov 6, 2020
…nits

DOC: Update list of functions that lose units
@rpsfonseca
Copy link
Contributor Author

Sorry for taking this long, but I opened a PR to address the confusing tips on CONTRIBUTING.md, as we talked about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs hacktoberfest-accepted PR that counts towards Hacktoberfest points for the author no-changelog-entry-needed units
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complete list of functions that lose units
5 participants