Repository navigation
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 |
ba0d0a5 to
3a369d6
Compare
3a369d6 to
32a9dbf
Compare
|
CC @preshanth |
|
This is looking good in my opinion. The examples are definitely avoiding defining the functions but instead setting values and I suppose that is intended. Are we going to make clear cut example in a different location ? |
32a9dbf to
e045d5d
Compare
|
Updated to fix a merge conflict. |
Expanding on |
e045d5d to
ee94eb4
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Hmm, only had a quick look, but my tendency would be not to include examples that are not physically meaningful; I basically do not see how a user is going to be helped by it. See, e.g., my immediate question inline...
Instead, I would suggest to add a more tutorial-like page to the docs where you construct a meaningful cosmologies from these traits. One could start with a De Sitter one and then add realism or so.
Of course, with such a page available you could refer to both LambdaCDM and that page in the docstrings.
A faster solution would be only to point to LambdaCDM in the docstring (haven't checked how readable that is).
|
|
||
| >>> @dataclasses.dataclass(frozen=True) | ||
| ... class ExampleHasBaryons(BaryonComponent): | ||
| ... Ob0: float |
There was a problem hiding this comment.
It seems annoying that you would have to define Ob0 when the trait already does so. Doesn't the order of traits imply an order here?
There was a problem hiding this comment.
It's not strictly necessary, just often good practice when making a dataclass to declare all fields for exactly the reason you mention: the order of traits implies the order of the fields. Being very explicit can be useful in that regard.
There was a problem hiding this comment.
This is an aspect of the abstract-final python pattern.
nstarman
left a comment
nstarman
left a comment
There was a problem hiding this comment.
Instead, I would suggest to add a more tutorial-like page to the docs where you construct a meaningful cosmologies from these traits. One could start with a De Sitter one and then add realism or so.
See #18876. With all these trait PRs making pedagogical cosmology classes is much simpler, but still not trivial. A de Sitter matter-only would be simpler than what is in #18876, but still verbose.
Hmm, only had a quick look, but my tendency would be not to include examples that are not physically meaningful; I basically do not see how a user is going to be helped by it. See, e.g., my immediate question inline...
My thought is to provide in the docstirng an immediate example of the API in use.
When constructing novel cosmologies the details of a de Sitter example are probably not going to be that relevant.
Per the use case of the traits, the only people that will see these docstrings are people that are constructing their own cosmology classes. For them I think the following things are very useful:
- tutorial docs. We do need to make these!
object.__dir__()to see the list of public methodsobject?(?)to see the docstring (& full code).
Right now when you are introspecting in an IDE you get no useful information about the API showing: a trivial construction, what methods are abstract and need overrides, and what is public API. These examples give all 3 useful classes of information.
Of course, with such a page available you could refer to both LambdaCDM and that page in the docstrings.
A faster solution would be only to point to LambdaCDM in the docstring (haven't checked how readable that is).
That uses FLRW so the person would need to work their way up the class hierarchy. Also FLRW is very large and complicated (for performance reasons) and it brings together all the traits. For the trait docstrings I want atomic examples.
I do think pointing to #18876 in the docstrings will be very useful!
|
|
||
| >>> @dataclasses.dataclass(frozen=True) | ||
| ... class ExampleHasBaryons(BaryonComponent): | ||
| ... Ob0: float |
There was a problem hiding this comment.
It's not strictly necessary, just often good practice when making a dataclass to declare all fields for exactly the reason you mention: the order of traits implies the order of the fields. Being very explicit can be useful in that regard.
@preshanth Yeah, that would be great to do in the docs. It's a little challenging because the traits are such surgical slices of a cosmology that this would probably have to be a narrative doc walking through building a full cosmology class. |
|
@mhvk, does it make sense that, given the audience that will be reading these docstrings, it's more important to communicate the API than to show a mathematically interesting cosmology. |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
github-project-automation
Bot
moved this from In Progress
to Done
in Cosmology, the Expansion
github-project-automation
Bot
moved this from Done
to In Progress
in Cosmology, the Expansion
nstarman
added
keep-open
and removed
Close?
|
@neutrinoceros as far as I can tell, the RTD failure is unrelated. |
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]> Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]> Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
ee94eb4 to
9e81bd6
Compare
|
@neutrinoceros, rebasing to pick up the new tests. |
Illustrative examples.
The target audience of these classes are devs, whom already have their intended physics and want illustrative examples when writing the code.