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

list of accepted meta data for custom Maps #1940

Closed
wants to merge 8 commits into from
Closed

Conversation

abit2
Copy link
Member

@abit2 abit2 commented Nov 7, 2016

#1832
updated the docstrings of the properties of what meta kwargs they use

@abit2
Copy link
Member Author

abit2 commented Nov 16, 2016

Ping @Cadair
Does it require more documentation?

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

As well as the inline comments the keyword and default values should be in double backticks i.e.

``detector``

I think we should also think of something more descriptive than 'meta keyword' I am open to suggestions but maybe 'obtained from meta keyword' as it's more than just a lookup in some cases.

Finally, I think this should be in the Notes section of the docstring so:

"""
Detector name

Notes
-----
Obtained from meta keyword: ``detector``, default: ``""``.

@@ -601,6 +655,7 @@ def rotation_matrix(self):
"""
Matrix describing the rotation required to align solar North with
the top of the image.
Meta keyword : PC1_1, PC1_2, PC2_1, PC2_2, CD1_1, CD1_2, CD2_1, CD2_2
Copy link
Member

Choose a reason for hiding this comment

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

some mention of of PC or CD would be nice here.

"""Heliographic longitude"""
"""
Heliographic longitude
Meta keyword : hgln_obs
Copy link
Member

Choose a reason for hiding this comment

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

for cases like this where a a not-None default is set it would be nice to have that in the docstring as well.

Copy link
Member

Choose a reason for hiding this comment

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

something like meta keyword: crval1 (default 0 deg), crval2 (default: 0 deg)

"""Heliographic latitude"""
"""
Heliographic latitude
Meta keyword : hglt_obs
Copy link
Member

Choose a reason for hiding this comment

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

this is not very clear, something like meta keyword: hglt_obs / crlt_obs / solar_b0 [in order of preference]

@Cadair
Copy link
Member

Cadair commented Jan 6, 2017

Also, all the files in sunpy/map/sources should be check to see if they modify any of these properties because then the docstrings should be changed there as well.

@Cadair Cadair added Documentation Affects the documentation map Affects the map submodule labels Jan 6, 2017
@abit2
Copy link
Member Author

abit2 commented Jan 7, 2017

didn't understand changing sunpy/map/sources part
can you please explain.

@abit2
Copy link
Member Author

abit2 commented Jan 10, 2017

@Cadair As far as I can tell, it doesnot change anywhere else in sunpy/map/sources

@Cadair
Copy link
Member

Cadair commented Jan 10, 2017

@abit2 I mean methods like this: https://github.com/sunpy/sunpy/blob/master/sunpy/map/sources/sdo.py#L51 also need documenting in the same fashion.

@Cadair
Copy link
Member

Cadair commented Jan 12, 2017

Sweet Jesus that's a lot of pep8

@OrkoHunter
Copy link

@abit2 I think the PR needs a rebase to ignore all the trivial E501s. :)

@Cadair
Copy link
Member

Cadair commented Jan 12, 2017

@OrkoHunter I thought it pulled the config from the main repo not the PR? Has that changed since the out-of-date version I was playing with?

@OrkoHunter
Copy link

OrkoHunter commented Jan 12, 2017

@Cadair Earlier it used to pull the config from the main repo's branch. But after testing on different projects, similar to Travis CI, pulling the (updated) config from the PR made more sense.

Now I think, there should be a change. If the config is not there in the PR's branch, it should check the main repo. I'd love to hear your suggestions.

@pep8speaks
Copy link

Hello @abit2! Thanks for updating the PR.

Line 1372:101: E501 line too long (138 > 100 characters)
Line 1415:101: E501 line too long (101 > 100 characters)
Line 1417:101: E501 line too long (101 > 100 characters)
Line 1583:101: E501 line too long (110 > 100 characters)

Line 8:1: E402 module level import not at top of file
Line 9:1: E402 module level import not at top of file
Line 10:1: E402 module level import not at top of file
Line 12:1: E402 module level import not at top of file
Line 13:1: E402 module level import not at top of file
Line 45:101: E501 line too long (154 > 100 characters)
Line 111:101: E501 line too long (112 > 100 characters)

Line 39:101: E501 line too long (110 > 100 characters)

Line 8:1: E402 module level import not at top of file
Line 9:1: E402 module level import not at top of file
Line 29:101: E501 line too long (121 > 100 characters)

Line 8:1: E402 module level import not at top of file
Line 9:1: E402 module level import not at top of file
Line 11:1: E402 module level import not at top of file
Line 47:101: E501 line too long (102 > 100 characters)

Line 8:1: E402 module level import not at top of file
Line 9:1: E402 module level import not at top of file
Line 10:1: E402 module level import not at top of file
Line 12:1: E402 module level import not at top of file
Line 13:1: E402 module level import not at top of file
Line 36:101: E501 line too long (119 > 100 characters)
Line 39:101: E501 line too long (144 > 100 characters)

Line 8:1: E402 module level import not at top of file
Line 9:1: E402 module level import not at top of file
Line 11:1: E402 module level import not at top of file
Line 12:1: E402 module level import not at top of file
Line 13:1: E402 module level import not at top of file
Line 15:1: E402 module level import not at top of file
Line 16:1: E402 module level import not at top of file
Line 17:1: E402 module level import not at top of file
Line 18:1: E402 module level import not at top of file
Line 112:101: E501 line too long (101 > 100 characters)
Line 179:101: E501 line too long (105 > 100 characters)

Line 8:1: E402 module level import not at top of file
Line 10:1: E402 module level import not at top of file
Line 11:1: E402 module level import not at top of file
Line 13:1: E402 module level import not at top of file
Line 14:1: E402 module level import not at top of file

Line 8:1: E402 module level import not at top of file
Line 9:1: E402 module level import not at top of file
Line 11:1: E402 module level import not at top of file

Line 8:1: E402 module level import not at top of file
Line 10:1: E402 module level import not at top of file
Line 11:1: E402 module level import not at top of file
Line 13:1: E402 module level import not at top of file
Line 14:1: E402 module level import not at top of file
Line 15:1: E402 module level import not at top of file
Line 41:101: E501 line too long (120 > 100 characters)

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this @abit2 it's looking really good!
Just a few sphinx formatting changes and stuff.


* First available:
1. ``ctype1``
2. ``HPLN-TAN``
Copy link
Member

Choose a reason for hiding this comment

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

These are defaults not keys

else with ``CD1_1``, ``CD1_2``, ``CD2_1``, ``CD2_2``.
.. rubric:: Metadata keywords:

* ``PC1_1``
Copy link
Member

Choose a reason for hiding this comment

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

These are generally written as PCi_j and CDi_j also these are an either or kinda thing, i.e.

* First Available
  1. `PCi_j`
  2. `CDi_j`

Returns the solar radius as measured by EIT in arcseconds.
.. rubric:: Metadata keyword:

* ``solar_r``
Copy link
Member

Choose a reason for hiding this comment

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

This is an actual operation on two keywords so we should probably write it as solar_r * cdelt1 to make it clear that's what it is.

dim=u.Quantity(self.dimensions),
scale=u.Quantity(self.scale),
tmf=TIME_FORMAT) + self.data.__repr__())
obs=self.observatory, inst=self.instrument, det=self.detector,
Copy link
Member

Choose a reason for hiding this comment

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

This change seems to mis-align these kwargs?

Image observation time
.. rubric:: Metadata keyword:

* ``date-obs`` Default: ``now``
Copy link
Member

Choose a reason for hiding this comment

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

Default 'now' to indicate it is a string.

wavelength of the observation
.. rubric:: Metadata keyword:

* ``wavelnth`` Default: 0
Copy link
Member

Choose a reason for hiding this comment

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

bacticks

Radius of the sun in meters
.. rubric:: Metadata keyword:

* ``rsun_ref`` Default: constants.radius
Copy link
Member

Choose a reason for hiding this comment

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

this can be a full object link

`sunpy.constants.radius`

1. ``rsun_obs``
2. ``solar_r``
3. ``radius``
4. Default: None
Copy link
Member

Choose a reason for hiding this comment

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

single backticks around None

Carrington longitude (crln_obs)
.. rubric:: Metadata keyword:

* ``crln_obs`` Default: None
Copy link
Member

Choose a reason for hiding this comment

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

backticks

@@ -605,6 +721,16 @@ def rotation_matrix(self):
"""
Matrix describing the rotation required to align solar North with
the top of the image.
.. rubric:: Metadata keywords:

* ``PC1_1``
Copy link
Member

Choose a reason for hiding this comment

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

These are an either or, and also normally written as PCi_j and CDi_j to cover all four. i.e it should be a first available with two entries.

@Cadair
Copy link
Member

Cadair commented Jan 12, 2017

@pep8speaks Keep Quiet

@Cadair Cadair self-assigned this Jan 28, 2017
@dpshelio dpshelio added this to Review in Docathon - 2017 Mar 6, 2017
@stale
Copy link

stale bot commented Jun 28, 2017

This pull request has been automatically marked as stale because it has not had any activity for the past five months. It will be closed if no further activity occurs. If the ideas in this pull request are still worth implementing, please make sure you open an issue to keep track of that idea!

@stale stale bot added the Stale The bot will close this PR after 6 months label Jun 28, 2017
@stale stale bot closed this Jul 28, 2017
@stale
Copy link

stale bot commented Jul 28, 2017

This pull request has been automatically closed since there was no activity for a month since it was marked as stale. If the ideas in this pull request are still worth implementing, please make sure you open an issue to keep track of that idea!

@nabobalis nabobalis added the Needs Adoption PRs that were abandoned but should be picked up again and merged in label Mar 27, 2018
@nabobalis nabobalis removed the Stale The bot will close this PR after 6 months label Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Affects the documentation map Affects the map submodule Needs Adoption PRs that were abandoned but should be picked up again and merged in
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants