Register format ascii.html to Cosmology.write - #13075

Hello @JefftheCloudDog 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2022-07-09 20:53:40 UTC

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@JefftheCloudDog the issue appears to be that the HTML table writer doesn't work with units.

Here's how I found the relevant documentation, which includes all the various things the HTML writer can do:

from astropy.cosmology import Planck18
tbl = Planck18.to_format("astropy.table")
tbl.writer.help("html")

While mildly annoying, there is an easy fix for the unit issue: every Parameter on a Cosmology stores the preferred unit, so you can convert each column to its unitless equivalent. This is a quick and dirty method, there's probably something cleaner.
Using Planck18 as an example:

import astropy.units as u
from astropy.cosmology import Planck18, Parameter
cosmo_cls = type(Planck18)
tbl = Planck18.to_format("astropy.table")

for n, col in zip(tbl.colnames, tbl.itercols()):  # Iter thru name and column
    param = getattr(cosmo_cls, n)  # Get parameter, and check it
    if not isinstance(param, Parameter) or param.unit in (None, u.one):
        continue
    # Replace column with unitless version
    tbl.replace_column(n, col.to_value(param.unit), copy=False)

tbl

@JefftheCloudDog the CI isn't fully running because the Code Style Check runs first and it's currently failing. There's usually an easy fix and that's to run tox -e codestyle from your terminal in the astropy folder. It can clean up most errors by itself. You might need to install tox.

conda activate [your env name]
conda install tox
cd ~/path/to/astropy
tox -e codestyle

I am a little stuck on the fix. From my testing, it appears that metadata is not stored properly as part of a cosmology object's metadata.

There are two fail cases:

1. read cosmology with metadata in table = cosmology object with metadata in columns with no metadata set
2. read cosmology without metadata in table = cosmology object with not metadata

Perhaps I am reading the documentation wrong, but is there a built in method for HTML that automatically converts a HTML metadata column to metadata? I was not able to find documentation about any such methods.

@JefftheCloudDog, a good observation. I just did some testing and it doesn't look like the table -> HTML writer keeps any of a table's metadata! Fixing this is out of the scope of this PR, so let's just make the Cosmology <-> HTML work as well as possible within these restrictions. Don't worry about a Cosmology's metadata. I think one thing to be careful about is the cosmology_in_meta argument of to_table.

@nstarman I set the cosmology_in_meta to be False for now because of the metadata issues. The tests should all pass now, I commented out the offending assert statements.

@JefftheCloudDog, it looks like the CI didn't start. Can you rebase and re-push? That should get it going. Thanks!

@nstarman I rebased and repushed. Looks like there are still some errors listed, like CI / Python 3.9 with all optional dependencies (pull_request) and Check PR change log / Check if towncrier change log entry is correct (pull_request_target)

@JefftheCloudDog If you click on the former it will take you to the full traceback, which is very useful for diagnosing problems. The changelog error is because this PR does not yet have a changelog. See https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#add-a-changelog-entry for details.

To get an easier diagnosis, try in the command line

>>> pytest -P cosmology -v -x --pdb

I updated init.py to include html as an import, looks like I am getting this circular import error regarding this line in init.py:
from . import cosmology, ecsv, html, mapping, model, row, table, yaml # noqa: F401, F403

and html.py:
from astropy.cosmology import Parameter

Seems like the error could be popping up because of the order of importing Parameter from astropy.cosmology, but rearanging the import order does not seem to fix this issue.

@JefftheCloudDog, please rebase on the main branch instead of merging.

@nstarman My mistake. I tried reverting back the previous commits and rebasing on main, but it looks like the feature branch is already up to date.

@nstarman I am a little new to rebasing, but I think I got it mostly figured out.

Locally, I tried reverting back to before the merge and rebasing with the main branch from there. I did modify a few more files regarding testing, such as astropy\cosmology\io\tests\test_.py to expect ascii.html so pytest takes in account of the new registered HTML table reader/writer.

I also want to note that some files were not added during the rebase, such as docs/changes/coordinates/13162.api.rst, which main does have. I am going to look into this. Such missing files are causing certain fails in the checks.

I also think there are rather a lot of commits at this point, and in the end, squashing might be required.

@nstarman The code should be updated. I added extra \ for some formatting errors. I amended the last commit for a minor fix. I think some squashing would be somewhat appropriate since the commit log is getting rather long.

There seems to be an issue with test_readwrite_html_mutlirow.
TypeError: only dimensionless scalar quantities can be converted to Python scalars
I'm not quite sure what this means. The other new tests work fine, and I commented out meta-related assertions since HTML meta is not implemented yet.

I am also still working on the TestReadWriteHTML tests and adding an entry to the rst. Is there an example of analogous tests for TestReadWriteHTML? Looks like there are no such tests for other test files in cosmology/io/tests.

There seems to be an issue with test_readwrite_html_mutlirow. TypeError: only dimensionless scalar quantities can be converted to Python scalars I'm not quite sure what this means. The other new tests work fine, and I commented out meta-related assertions since HTML meta is not implemented yet.

This error occurs when a quantity with units is converted to a python scalar. On your machine try float(10 * u.km) and it should raise this error.

Looking at the log, it appears that some units aren't being stripped. For instance, I looked and saw error messages like this:
astropy.units.core.UnitConversionError: 'eV' (energy/torque/work) and '' (dimensionless) are not convertible

The source of this problem is that HTML tables don't support units. Recall in the PR there is

# Replace column with unitless version
table.replace_column(n, (col << param.unit).value, copy=False)

to strip units. This is applied when calling cosmo.write(...), because it's part of the Cosmology I/O, but in test_readwrite_html_mutlirow a multi row table is made and Table's I/O is used.

The solution is to for-loop over the combined table and remove units, like is done above.

If you are interested in a followup PR, you could add a flag to to_table like remove_units so that ``cosmo.to_format("astropy.table", remove_units=True) makes a table that can be written to HTML.
e.g.

tbl = cosmo.to_format("astropy.table", remove_units=True)
tbl.write("ascii.html")

In the meantime, just remove the units in test_readwrite_html_mutlirow.

I am also still working on the TestReadWriteHTML tests and adding an entry to the rst. Is there an example of analogous tests for TestReadWriteHTML? Looks like there are no such tests for other test files in cosmology/io/tests.

The ECSV I/O is a good example of read/write tests
(https://github.com/astropy/astropy/blob/main/astropy/cosmology/io/tests/test_ecsv.py).

Also when writing tests, take a look at the code coverage. That's the best way to see what needs a test!

Thanks @JefftheCloudDog for all the hard work! I squashed the PR and made a few necessary changes so this could get into v5.2.

@mhvk, I contributed heavily to this PR, so a second set of eyes would be appreciated. Thanks!

CI passed, so I pushed the hopefully last small doc change. GTG?