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

Some non-functioning-APIs fixed #2019

Merged
merged 11 commits into from
Mar 1, 2014
Merged

Some non-functioning-APIs fixed #2019

merged 11 commits into from
Mar 1, 2014

Conversation

krngrvr09
Copy link

26 out of the 36 non-functioning APIs in 'whatsnew' folder are fixed. #1221

@@ -12,7 +12,7 @@ package with a unified installer, including:

* asciitable as :mod:`astropy.io.ascii`
* PyFITS as :mod:`astropy.io.fits`
* votable as :mod:`astropy.io.vo`
* votable as :mod:`astropy.io.votable`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of a question for the wider audience: I wonder if we should retroactively update these... It certainly was correct at the time. In this specific case, it probably doesn't matter, but for someone considering updating from 0.1 to 0.2, when 0.3 is already released, this kind of thing could add confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

And to clarify -- I wouldn't be opposed to this changing to:

``astropy.io.vo``

i.e., a non-link, in order to make nitpicky mode happy.

EDIT: Replaced "would" with "wouldn't" above

Copy link
Member

Choose a reason for hiding this comment

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

I also agree that we shouldn't change the name of the module in the 0.1.rst file - it should reflect what the code was at the time.

@mdboom - what do you suggest we should use instead of double backticks for cases like this? Should we just leave the single backticks and live with the warnings?

Copy link
Member

Choose a reason for hiding this comment

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

To some extent, this applies to any other renaming - why not use the double backticks to refer to old names that no longer resolve?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops: I mistyped above. I meant "wouldn't be opposed" rather than "would be opposed." Sorry about that ;)

So I agree that double backticks are probably the best solution here -- it will resolve the warning without renaming it for the reader.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sounds good :)

@krngrvr09 - could you change this example to:

``astropy.io.vo``

rather than rename it?

Copy link
Member

Choose a reason for hiding this comment

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

👍

@mdboom
Copy link
Contributor

mdboom commented Jan 29, 2014

Aside from my question (which is really just a policy question about changelogs), this looks good. Thanks for doing this kind of work: it really enhances the overall polish.

@@ -240,7 +242,7 @@ return Numpy arrays.

As shown above, all the coordinate classes have now been renamed to drop the
``Coordinates`` suffix (e.g. ``ICRS`` instead of ``ICRSCoordinates``). In
addition, `HorizontalCoordinates` has now been renamed to `AltAz`.
addition, ``HorizontalCoordinates`` has now been renamed to `astropy.coordinates.builtin_systems.AltAz`.
Copy link
Member

Choose a reason for hiding this comment

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

I think there should be a ~ in the link for AltAz, i.e. ~astropy.coordinates....

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't seem to make a difference. The absence of '', seemed unusual to me too, but since it was not showing a warning, I let it go. What does the '' signify?

Copy link
Member

Choose a reason for hiding this comment

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

Without the ~, you should see 'astropy.coordinates.builtin_systems.AltAz' and with it, you should see 'AltAz' - i.e. when you add it, you say that you only want to see the last part of the path.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, alright. Since I was checking only the warning, I didn't realize that. Thank you.

@astrofrog
Copy link
Member

@krngrvr09 - I just saw you added some commits - in general once you've resolved issues we raised, add a comment otherwise GitHub doesn't tell us there have been updates to the pull request.

@astrofrog
Copy link
Member

@krngrvr09 - this looks good now - shall I merge it, or do you want to add more fixes first?

@@ -12,7 +12,7 @@ package with a unified installer, including:

* asciitable as :mod:`astropy.io.ascii`
* PyFITS as :mod:`astropy.io.fits`
* votable as :mod:`astropy.io.vo`
* votable as :mod:``astropy.io.vo``
Copy link
Member

Choose a reason for hiding this comment

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

This should drop the :mod: directive too. It should just read

* votable as ``astropy.io.vo``

Copy link
Author

Choose a reason for hiding this comment

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

Just so I know, what does the :mod: do?

Copy link
Author

Choose a reason for hiding this comment

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

Also, build_sphinx is showing, astropy.io.fits as broken. Any idea why?

@embray
Copy link
Member

embray commented Feb 3, 2014

I just added a couple more suggestions, but other than that this is good.

@krngrvr09
Copy link
Author

@astrofrog Almost everything is fixed.
astropy.cosmology.core.Cosmology, is broken, should I add single ticks?

@astrofrog
Copy link
Member

@krngrvr09 - yes, could you add single ticks? The link will still be broken (I will raise this issue separately) but single ticks is the right thing in this case.

@krngrvr09
Copy link
Author

Broken links of the whatsnew folder are fixed. Currently working on 'wcs'.

@astrofrog
Copy link
Member

@krngrvr09 - will you push that here too, or would you prefer to open a new pull request?

@krngrvr09
Copy link
Author

I think I'll push them here. After fixing 3-4 modules, you can merge them.

@krngrvr09
Copy link
Author

@astrofrog 'vo' and 'wcs' modules are fixed now.
Few questions:

  1. 'astropy.io.fits' and 'astropy.io.votable.validator.result.Result' are broken. They shouldn't be. Why is there not a link for them in the docs?
  2. I have adjusted the formatting of a docstring. I believe, I shouldn't have pushed that in this PR. But apparently, 'git commit .' pushes the commits that were not even staged. (Lesson learnt).
  3. Is it good, if you merge after I fix, 'utils', 'units', and 'time'?

@astrofrog
Copy link
Member

@krngrvr09 ignore the fact that 'astropy.io.fits' and 'astropy.io.votable.validator.result.Result' don't work for now. For the latter, I think @embray can fix that, and for the latter we either need to document the Result class, or @mdboom or @pllim will need to remove the references to the classes. The links themselves are correct and don't need fixing.

For the accidental change to wcs.py, just unindent the code again (it should be as it was originally) and commit a fix.

Once you've fixed the ones you mention, we can merge - thanks!

@pllim
Copy link
Member

pllim commented Feb 10, 2014

I thought we already discussed astropy.io.votable.validator.result.Result in the past. We decided: It is used by astropy.vo.validator but users do not need to know more than that, so it's okay to just leave it mentioned by undocumented. Has that changed?

@mdboom
Copy link
Contributor

mdboom commented Feb 10, 2014

@pllim: You're exactly right. I think we just need to not refer to it from vo/validator.rst... Maybe there's a way to reword that paragraph?

@pllim
Copy link
Member

pllim commented Feb 10, 2014

Will using double quote thingy suffice? Like

``astropy.io.votable.validator.result.Result``

@mdboom
Copy link
Contributor

mdboom commented Feb 10, 2014

@pllim: That's probably fine, but it seems a little rude to refer to an undocumented class like that. (Like a dictionary that for a definition says "see X" but then doesn't define X). But I'm actually not sure what you mean by "original attribute names" -- do you mean the dictionary keys? Maybe we should just list what they are?

@pllim
Copy link
Member

pllim commented Feb 11, 2014

@krngrvr09 and @astrofrog , astropy.vo doc was fixed in #2071 . FYI.

@@ -20,8 +20,8 @@ out only standard keywords), in accordance with `Postel's prescription
Header-reading relaxation constants
-----------------------------------

`~astropy.wcs.WCS`, `~astropy.wcs.Wcsprm` and
`~astropy.wcs.find_all_wcs` have a *relax* argument, which may be
`~astropy.wcs.wcs.WCS`, `~astropy.wcs.Wcsprm` and
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a tricky one. The actual place where we want users to import astropy.wcs.wcs.WCS from is astropy.wcs.WCS, but the documentation generates it for its "real" location. I know that there's some work going on to fix this problem in #1826 and elsewhere...

@eteq: Since you're much more involved with all this than I am, can you comment? Would it make sense for @krngrvr09 to hold off on this for now until #1826 is resolved. I'm just worried if we make this change now, we'll just have to change it back to what we really want it to be later...

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, good point @mdboom. But there are a lot of places where we've been doing this already, so we'll have to go correct it all en masse in the future.

I guess if you think this will be merged soon, I'd say go ahead and accept this, as it might still take more time for #1826 to get working (lots of Sphinx black magic going on there...). We'll probably want to try to find an automated way to take care of it anyway after #1826...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, if #1826 is still a ways off...

@mdboom
Copy link
Contributor

mdboom commented Feb 12, 2014

I just looked over the wcs part of this. For the most part, it totally makes sense. I'm not sure how best to handle the APIs that currently say what we want them to, but currently break because Sphinx wants to document the classes in their source location rather than their API location. Hopefully that's resolvable, but I think @eteq and @kbarbary will have more insight into that than I...

@krngrvr09
Copy link
Author

@astrofrog Done. Unsure about a few things.

  1. [units.equivalencies.rst]Couldn't find the forward and backward function. As far as I have researched, they were originally a part of pyparsing. But now we are no longer using pyparsing(right?). So, should I leave it as it is, put double ticks, or will it be eventually removed from the docs. (Correct me, if I'm wrong about anything,)
  2. [units.equivalencies.rst] find_equivalent_units method is in the class EquivalentUnitsList. In the docs it astropy.units.core.Unit.find_equivalent_units. Is this an error?
  3. Couldn't find a link to astropy.utils.compat.

@mdboom I have not changed anything in wcs/relax.rst. Let me know if something is required.

P.S - 300 broken links fixed!

@astrofrog
Copy link
Member

@krngrvr09 - in the following sentence:

`from_unit` and `to_unit` are the equivalent units.  `forward` and
`backward` are functions that convert values between those units.

all the ones in single ticks should be in double ticks. These are just made up variable names for what an equivalency should consist of.

For the find_equivalent_units, the current link is correct:

In [3]: Unit('m').find_equivalent_units()
Out[3]: 
  Primary name | Unit definition | Aliases     
[
  AU           | 1.49598e+11 m   | au           ,
  Angstrom     | 1e-10 m         | AA, angstrom ,
  cm           | 0.01 m          | centimeter   ,
  lyr          | 9.46073e+15 m   | lightyear    ,
  m            | irreducible     | meter        ,
  micron       | 1e-06 m         |              ,
  pc           | 3.08568e+16 m   | parsec       ,
  solRad       | 6.95508e+08 m   | R_sun, Rsun  ,
]

the issue is that indeed it does not appear in the docs:

http://docs.astropy.org/en/stable/api/astropy.units.core.Unit.html#astropy.units.core.Unit

maybe @mdboom or @eteq can look into why?

@astrofrog
Copy link
Member

See the note at the top of http://docs.astropy.org/en/stable/utils/index.html for astropy.utils.compat. I think you should use double backticks.

@krngrvr09
Copy link
Author

Oh yes. I had read that note. Somehow forgot. My bad!

@krngrvr09
Copy link
Author

@astrofrog Is there anything else, I have to do?

@@ -253,9 +253,9 @@ of ``c`` is used instead of the constant.
Displaying available equivalencies
----------------------------------

The :meth:`~astropy.units.core.Unit.find_equivalent_units` method also
The :meth:`~astropy.units.core.UnitBase.find_equivalent_units` method also
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm: this looks like the correct change to me.

@astrofrog
Copy link
Member

@krngrvr09 - we should rebase this (without squashing) to get rid of the merge commits here. I can send instructions on doing that later this evening.

@astrofrog
Copy link
Member

@krngrvr09 - I made a backup of your branch in case anything goes wrong. Just do:

git fetch upstream
git rebase -i upstream/master

Don't edit the commits, just save and continue and it will start the rebase. I think you will get a conflict, so try and resolve it and then proceed. I have a backup of the branch so as a last option I can merge it manually - but it'd be good to learn to rebase.

There are some instructions here:

http://docs.astropy.org/en/stable/development/workflow/development_workflow.html#rebase-on-trunk

If you have any questions, feel free to ask here or see if anyone is on IRC.

For future, if you want to update the commits in your branch, don't merge master into the branch - instead, rebase of the upstream master to always include your commits at the top.

@krngrvr09
Copy link
Author

Successfully rebased. On git diff master, I see all my commits. Should I push or force push?

@astrofrog
Copy link
Member

@krngrvr09 - you'll need to force push for the changes to appear here.

@krngrvr09
Copy link
Author

@astrofrog Done.

@astrofrog
Copy link
Member

@krngrvr09 - great job with the rebasing :) This looks good now, let's just wait on Travis.

@astrofrog astrofrog added this to the v0.4.0 milestone Feb 27, 2014
@krngrvr09
Copy link
Author

All thanks to you. :)

On Fri, Feb 28, 2014 at 2:28 AM, Thomas Robitaille <notifications@github.com

wrote:

@krngrvr09 https://github.com/krngrvr09 - great job with the rebasing
:) This looks good now, let's just wait on Travis.

Reply to this email directly or view it on GitHubhttps://github.com//pull/2019#issuecomment-36290577
.

@astrofrog
Copy link
Member

@krngrvr09 - I just realized that two or three of the API links may have to be changed back now that #1826 has been merged. Now that you have rebased you should have these changes so if you run the build_sphinx again, would you mind double check if any of the fixes included here have re-appeared as warnings?

@krngrvr09
Copy link
Author

I'll do that right away.

On Fri, Feb 28, 2014 at 2:31 AM, Thomas Robitaille <notifications@github.com

wrote:

@krngrvr09 https://github.com/krngrvr09 - I just realized that two or
three of the API links may have to be changed back now that #1826https://github.com/astropy/astropy/pull/1826has been merged. Now that you have rebased you should have these changes so
if you run the build_sphinx again, would you mind double check if any of
the fixes included here have re-appeared as warnings?

Reply to this email directly or view it on GitHubhttps://github.com//pull/2019#issuecomment-36290952
.

@krngrvr09
Copy link
Author

@astrofrog Broken links have gone up to 2600.

astrofrog added a commit that referenced this pull request Mar 1, 2014
Some non-functioning-APIs fixed
@astrofrog astrofrog merged commit d34d53f into astropy:master Mar 1, 2014
@astrofrog
Copy link
Member

@krngrvr09 - thanks for the contribution!

@krngrvr09 krngrvr09 deleted the broken_api branch March 1, 2014 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants