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.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Thanks for the PR! Overall I think this looks very good. The Quantity code is performance critical. Can you provide some timing tests showing the performance impact. Thanks and happy new year 🎆

Thanks for review - I am happy the code is good overall

I am working on it (using asv) and will update the PR shortly

Have a nice start of the year 😊

Hi! @nstarman, for the past day, I have been working to provide timing tests for my code. Here is how I did it. I wonder if I did the right thing:

• Used asv for benchmarking

• I started with asv run HASHFILE:hashestobenchmark.txt. This file has 2 hashes: 6156e3a (points to the current astropy repo state) and d08e712 (points to my commit)

• After this, asv compare 6156e3ad d08e7120 -s > compare.csv and stored results in a csv file (below)
  compare.csv

• I also used compare like this, which gives somewhat unusual results (I think it should have been the other way around for the second command):

My inference is that the code is working fine, but I am not sure. I would be glad if you could suggest what actions can be taken further

Have a nice day 😉

Hi @kartavayv, that's a good way to do it, but even doing %timeit in a REPL would be informative and show a direct numerical comparison. Also, this is a distinct enough new feature, impacting a separate subpackage of Astropy that it's probably a good idea to split this into a separate PR, preceding this one. I think @mhvk would be interested in this.

Hi @nstarman, thanks for helping me out! (can't believe I am talking to real astrophysicists!!!!)

I checked for the potential overhead by benchmarking Quantity(str) for non-LaTeX input. Here are the results:

main : 4.62 μs ± 110 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

add-latex-reader : 4.6 μs ± 65.3 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

so, I think it is working perfectly fine, suggesting no regression for normal string parsing

Regarding the split suggestion: if you think it's preferable, I am happy to do so!

@nstarman should I make some improvements in this...!!!?

@kartavayv, sorry for the delay.
That's great that there's no regression! Given that, yes, I think you should make a PR to units that would precede this PR. 🚀

Hey @nstarman , from what I can understand, you are asking me to make changes in the units because there is no inverse parser from LaTeX -> Units ...? I think it's probably because my code uses a workaround del table[0] instead of solving the actual problem of not having this parsing facility (I am working on this). Am I thinking in the right direction....?

Thanks for your time 😊

Hi @kartavayv! Actually, I'm just asking for you to cherrypick your units-related code changes and make a new PR to astropy.units with just those. They are worth considering independently and first, before this PR :).

Hi @nstarman, just opened a new PR (just like you said 😊). I have also rebased this PR onto that (#19214) "new" PR...

If there are any other improvements to be made, I am ready; it's getting intense and exciting!!!

Rebased this branch back onto main, latex branch, and units-quantity-parsing branch are made for different purposes, so it's not a good idea to rebase one onto another!

@nstarman - Made some changes, just like you said (very helpful!). I think this will also help in the future n-D tensor parsing.

Performance Stats

add-latex-reader

• u.Quantity("78m") : 15.3 μs ± 306 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
• u.Quantity(np.array([7,8,9])): 6.01 μs ± 225 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

main

• u.Quantity("78m") : 15.5 μs ± 627 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
• u.Quantity(np.array([7,8,9])) : 6.11 μs ± 377 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

What was the specific error related to mandatory units?

Some of the generalized test functions that are failing:

• astropy/cosmology/_src/tests/io/test_connect.py::test_readwrite_complete_info because of got = Cosmology.read(fname, format=format) (97)

• astropy/cosmology/_src/tests/io/test_connect.py::test_readwrite_from_subclass_complete_info because of got = cosmo_cls.read(fname, format=format) (119)

• Some errors are coming from: astropy/cosmology/_src/tests/io/base.py: because of return self.functions["read"](*args, **kwargs) (192)

Not using units inside these functions makes read_latex() raise TypeError, and if used, I am not sure which other formats will raise it

These seem like generalized functions for various formats, so adding units will probably mess up tests for other formats

@nstarman - is there a better way to do it....? Something I might be missing...?

got = Cosmology.read(fname, format=format)

Ok. Got it. Yes, this is a good point. It's not that the method with required kwargs units doesn't work, it's that the signature of the function is different from the rest of the registered formats, so it's hard to build good generalized test code.

Some things I'm seeing.

1. latex shouldn't be tested intest_readwrite_complete_info or test_readwrite_from_subclass_complete_info since it doesn't have complete info! We need to update the test logic so that it skips formats with incomplete info. The strategy is to update

# Collect the registered read/write formats.
#   (format, supports_metadata, has_all_required_dependencies)
readwrite_formats = [
    ("ascii.ecsv", True, True),
    ("ascii.html", False, HAS_BS4),
    ("ascii.latex", False, True),
    ("json", True, True),
    ("latex", False, True),
]

to include a 4th elt in each tuple which is "supports units". So ("latex", False, False, True),.
Then the test can be updated to something like

        rkw = {"format": format}
        if not supports_units:
            rkw |= {"units": ...}
        # read with the same class that wrote.
        got = cosmo_cls.read(fname, **rkw)
        assert got == cosmo
        assert (not metaio) ^ (dict(got.meta) == dict(cosmo.meta))

        # this should be equivalent to
        got = Cosmology.read(fname, **rkw, cosmology=cosmo_cls)
        assert got == cosmo
        assert (not metaio) ^ (dict(got.meta) == dict(cosmo.meta))

        # and also
        got = Cosmology.read(fname, **rkw, cosmology=cosmo_cls.__qualname__)
        assert got == cosmo
        assert (not metaio) ^ (dict(got.meta) == dict(cosmo.meta))

2. Then it would be good to add a completely separate test showing that passing in the wrong units makes it so that the cosmology doesn't round-trip properly through a latex write-read cycle.

It looks like the test failures are only small changes to the docs:

____________________________ [doctest] builtin.rst _____________________________
071 
072 
073 *************************************
074 Built-in Cosmology Read/Write Formats
075 *************************************
076 
077 To see a list of the available read/write file formats:
078 
079     >>> from astropy.cosmology import Cosmology
080     >>> Cosmology.write.list_formats()
Differences (unified diff with -expected +actual):
    @@ -3,5 +3,4 @@
      ascii.ecsv  Yes   Yes           Yes
      ascii.html  Yes   Yes           Yes
    -ascii.latex   Yes   Yes   Yes
    -ascii.latex   Yes   Yes   Yes
    -ascii.mrt     Yes   Yes   Yes
    +ascii.latex  Yes   Yes           Yes
    +  ascii.mrt  Yes   Yes           Yes

/home/runner/work/astropy/astropy/docs/cosmology/io/builtin.rst:80: DocTestFailure
_____________________________ [doctest] index.rst ______________________________
051 information (see :ref:`cosmology_io_details`). However using the base class is safest as
052 it provides no default information and therefore requires the file to have all necessary
053 information to describe a cosmology.
054 
055 To see a list of the available read/write file formats:
056 
057 .. code-block:: python
058 
059     >>> from astropy.cosmology import Cosmology
060     >>> Cosmology.read.list_formats()
Differences (unified diff with -expected +actual):
    @@ -1,4 +1,6 @@
    -  Format   Read Write Auto-identify
    ----------- ---- ----- -------------
    -ascii.ecsv  Yes   Yes           Yes
    -ascii.html  Yes   Yes           Yes
    +   Format   Read Write Auto-identify
    +----------- ---- ----- -------------
    + ascii.ecsv  Yes   Yes           Yes
    + ascii.html  Yes   Yes           Yes
    +ascii.latex  Yes   Yes           Yes
    +  ascii.mrt  Yes   Yes           Yes

@nstarman - sorryy for this very long delay!

• Reader is now working fine. Having different interconvertible units makes the values change. This was not possible with the previously approved changes:
  In: cosmo = Cosmology.read(file, format="ascii.latex", units={"H0": u.km/(u.mpc*u.s),"Tcmb0":u.mK, "m_nu":u.eV} )
Out: FlatLambdaCDM(name='Planck18', H0=<Quantity 6.766e+10 km / (Mpc s)>, Om0=0.30966, Tcmb0=<Quantity 0.0027255 K>, Neff=3.046, m_nu=<Quantity [0. , 0. , 0.06] eV>, Ob0=0.04897)

• I had to make units={} instead of units: ..." in the tests you suggested because it was causing the failure down the line in this part of table.py (941) :

 if len(values) != len(self.columns):
                raise ValueError(
                    f"sequence of {attr} values must match number of columns"
                ): 

• I added Latex round-tripping cycle test to test_latex.py i wonder if that was to be added somewhere in test_connect.py
• I am dubious about readwrite_formats of test_connect.py in the new commit. I am not sure if that is what should be there.
• This also makes units a mandatory arg, as discussed above

I haven't made last arg in (latex", False, True, False) , True as you have suggested. I am confused by two latexes in readwrite_formats I wonder which one to make True

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month 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.

Updated Context

• Reader code is updated according to the new generic syntax (as @neutrinoceros used for the write method)
• Found that the logic of using if not supports_units was inverted, which was putting units kwarg for the wrong readers down the chain, and then it had to be cleaned up by explicitly removing it. The new fix doesn't require that.

I think this is ready for merge!

@dhomeier - can you look into this?

I checked for CI failures; it's affecting almost all new PRs...

CI should be fixed now. Please rebase.