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
Consolidate angle modules #15220
Consolidate angle modules #15220
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes a lot of sense to consolidate this. Just a few comments, of my typical don't-change-more-than-strictly-needed-for-renaming type.
This should wait. I am trying to remove some deprecated stuff in another PR. There will be conflicts. |
6a54389
to
5775843
Compare
Hmm. Regenerating lex and parse tab broke many things. |
20ea40b
to
1d3ac1c
Compare
@pllim @mhvk @eerovaher, I'm having trouble tracking which |
RTD log contains the line
This pull request:
It looks like the change log entry is causing the problem. |
|
009c22e
to
c69bfe1
Compare
Failures are unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have enough code dealing with angles that creating a separate directory is a good idea, but I have a few questions and remarks about some details.
|
||
Angle-related classes and functions have been consolidated into the module ``angles``. | ||
In particular, ``angle_utilties`` have been moved to ``angles.util``, | ||
and select errors have been moved to ``angles.errors``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the users are supposed to import everything from astropy.coordinates
then I don't see why we would even want to mention angles
, let alone angles.utils
or angles.errors
. We should just state that there has been some refactoring that might break some imports from some modules within astropy.coordinates
, but everything that could be imported from astropy.coordinates
itself still can be, and what cannot be imported from astropy.coordinates
is and has been considered private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, you're leaning towards "private" module (from opening comment). Happy to do that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with @eerovaher - let's stick with the current astropy scheme where users are supposed to import only from submodules, not below. Arguably, this is just a refactoring that does not even need a changelog entry. But I do think it is a good idea to have one anyway, if we phrase it along the lines @eerovaher suggested. It is also fine to be friendly and note that if people use stuff that is not exposed on top, they should feel welcome to submit a feature request to make it public -- since then obviously it is useful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; agree with @eerovaher (and you) to import from astropy.coordinates
wherever it is possible to do so. Otherwise, just nits.
This comment was marked as resolved.
This comment was marked as resolved.
70825b9
to
8082f6b
Compare
Resolved! It just required a rebase. |
Linkcheck failure unrelated. |
It looks like the only reason this can't be merged is that there are merge conflicts. |
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
8082f6b
to
8f90315
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
@eerovaher, rebased! |
A new sub-module of coordinates that is a consolidation of all the various angle stuff.
IMO this should be a public module, but I could go either way! Everything can just be imported up to
astropy/coordinates
.Use Squash and Merge!