Repository navigation
Conversation
Contributor
|
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.
|
Contributor
|
👋 Thank you for your draft pull request! Do you know that you can use |
nstarman
requested changes
Jun 2, 2023
nstarman
left a comment
nstarman
left a comment
Member
There was a problem hiding this comment.
Thanks @Mubin17 for the PR. Most things look good. I have just 2 corrections"
- the format should be "ascii.mrt", in line with how it's registered with Table
- with
represent_mixins_as_columnsdo we still need to make separate neutrino columns?
Comment on lines
+102
to
+107
| m_nu = table_main["m_nu"] | ||
| table.remove_column("m_nu") | ||
| for i in range(len(m_nu[0])): | ||
| column_name = f"mnu{i}" | ||
| column_data = m_nu[0][i] | ||
| table.add_column(column_data, name=column_name, index=-2) |
Member
There was a problem hiding this comment.
With represent_mixins_as_columns(table_main), does this serialize without breaking apart the neutrino mass?
Contributor
Author
There was a problem hiding this comment.
I may be completely wrong in my understanding of how serializing is supposed to work in this context. But I believe represent_mixins_as_columns(table_main), does not serialize the table without breaking apart the neutrino mass, and here is why:
from astropy.cosmology import Planck18
from astropy.table.serialize import represent_mixins_as_columns
table_main = Planck18.to_format("astropy.table")
table = represent_mixins_as_columns(table_main)
m_nu_table_main = table_main['m_nu'] # Get the m_nu column from table_main
m_nu_table = table['m_nu'] # Get the m_nu column from table
N = len(m_nu_table[0]) # Get the number of elements in m_nu
for i in range(N):
column_data_table = m_nu_table[0][i]
print(column_data_table, type(column_data_table))
for i in range(N):
column_data_table_main = m_nu_table_main[0][i]
print(column_data_table_main, type(column_data_table_main))Output:
0.0 <class 'numpy.float64'>
0.0 <class 'numpy.float64'>
0.06 <class 'numpy.float64'>
0.0 eV <class 'astropy.units.quantity.Quantity'>
0.0 eV <class 'astropy.units.quantity.Quantity'>
0.06 eV <class 'astropy.units.quantity.Quantity'>
The way I see it is that, for serialization to work correctly the outputs must be the same. And this is the primary reason why I have two separate variables table_main and table
Member
There was a problem hiding this comment.
You're right that represent_mixins_as_columns doesn't help here. I'll open an issue to see if we can extend represent_mixins_as_columns to make structured arrays into a series of columns.
Member
|
@Mubin17, the tests are failing. It looks like many are related to this PR. If you need help, feel free to ping me for questions. |
Contributor
Author
|
@nstarman do have any insight on what is triggering this error, self = <astropy.cosmology.tests.test_core.TestCosmology object at 0x1186f38b0>, read = <astropy.cosmology.connect.CosmologyRead object at 0x118e04c40>
write = <astropy.cosmology.connect.CosmologyWrite object at 0x118e06140>
tmp_path = PosixPath('/private/var/folders/t3/4lbhlq652z16wh1ldqsfr04m0000gn/T/pytest-of-mubinmanasia/pytest-8/test_to_mrt_bad_index0')
def test_to_mrt_bad_index(self, read, write, tmp_path):
"""Test if argument ``index`` is incorrect"""
fp = tmp_path / "test_to_mrt_bad_index.mrt"
> write(fp, format="ascii.mrt")
cosmology/io/tests/test_mrt.py:25:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
cosmology/connect.py:126: in __call__
self.registry.write(self._instance, *args, **kwargs)
io/registry/core.py:383: in write
return writer(data, *args, **kwargs)
cosmology/io/mrt.py:127: in write_mrt
cols, names = tuple(zip(*((m, f"m_nu[{i}]") for i, m in enumerate(m_nu[0]))))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <Quantity 0.6 eV>
def __iter__(self):
if self.isscalar:
> raise TypeError(
f"'{self.__class__.__name__}' object with a scalar value is not"
" iterable"
)
E TypeError: 'Quantity' object with a scalar value is not iterable
units/quantity.py:1275: TypeErrorThe same error is triggered whenever the write_mrt() method is called in tests FAILED cosmology/tests/test_core.py::TestCosmology::test_to_mrt_bad_index - TypeError: 'Quantity' object with a scalar value is not iterable
FAILED cosmology/tests/test_core.py::TestCosmology::test_to_mrt_cls[QTable] - TypeError: 'Quantity' object with a scalar value is not iterable
FAILED cosmology/tests/test_core.py::TestCosmology::test_to_mrt_cls[Table] - TypeError: 'numpy.float64' object is not iterable
FAILED cosmology/tests/test_core.py::TestCosmology::test_readwrite_mrt_instance - TypeError: 'Quantity' object with a scalar value is not iterable
FAILED cosmology/tests/test_core.py::TestCosmology::test_readwrite_mrt_subclass_partial_info - TypeError: 'Quantity' object with a scalar value is not iterable
FAILED cosmology/tests/test_core.py::TestCosmology::test_readwrite_complete_info[ascii.mrt-False-True] - TypeError: 'Quantity' object with a scalar value is not iterable
FAILED cosmology/tests/test_core.py::TestCosmology::test_readwrite_from_subclass_complete_info[ascii.mrt-False-True] - TypeError: 'Quantity' object with a scalar value is not iterable
|
Member
|
So this is my fault. If you look at m_nu can be a scalar, which doesn't work with the code I pushed. m_nu needs to be listified first. |
Contributor
Author
|
Thanks for the insight @nstarman I've listified the m_nu parameter in the There is one last set of failing tests that needed to be fixed: self = <astropy.cosmology.flrw.tests.test_wpwazpcdm.TestwpwaCDM object at 0x12b6f0ac0>, cosmo_cls = <class 'astropy.cosmology.flrw.wpwazpcdm.wpwaCDM'>
cosmo = wpwaCDM(name="ABCMeta", H0=70.0 km / (Mpc s), Om0=0.27, Ode0=0.73, wp=-0.9, wa=0.2, zp=0.5 redshift, Tcmb0=3.0 K, Neff=3.04, m_nu=[0. 0. 0.] eV, Ob0=0.03)
read = <astropy.cosmology.connect.CosmologyRead object at 0x1693be8c0>, write = <astropy.cosmology.connect.CosmologyWrite object at 0x1693bfa00>
tmp_path = PosixPath('/private/var/folders/t3/4lbhlq652z16wh1ldqsfr04m0000gn/T/pytest-of-mubinmanasia/pytest-28/test_readwrite_mrt_instance9')
def test_readwrite_mrt_instance(
self, cosmo_cls, cosmo, read, write, tmp_path
):
"""Test cosmology -> ascii.mrt -> cosmology."""
fp = tmp_path / "test_readwrite_mrt_instance.mrt"
# ------------
# To Table
write(fp, format="ascii.mrt")
# some checks on the saved file
tbl = Table.read(fp, format="ascii.mrt")
assert tbl["name"] == cosmo.name
# ------------
# From Table
tbl["mismatching"] = "will error"
tbl.write(fp, format="ascii.mrt", overwrite=True)
# tests are different if the last argument is a **kwarg
if tuple(cosmo._init_signature.parameters.values())[-1].kind == 4:
got = read(fp, format="ascii.mrt")
assert got.__class__ is cosmo_cls
assert got.name == cosmo.name
assert "mismatching" not in got.meta
return # don't continue testing
# read with mismatching parameters errors
with pytest.raises(TypeError, match="there are unused parameters"):
read(fp, format="ascii.mrt")
# unless mismatched are moved to meta
> got = read(fp, format="ascii.mrt", move_to_meta=True)
cosmology/io/tests/test_mrt.py:89:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
cosmology/connect.py:86: in __call__
cosmo = self.registry.read(self._cls, *args, **kwargs)
io/registry/core.py:218: in read
data = reader(*args, **kwargs)
cosmology/io/mrt.py:77: in read_mrt
return from_table(
cosmology/io/table.py:138: in from_table
return from_row(row, move_to_meta=move_to_meta, cosmology=cosmology)
cosmology/io/row.py:80: in from_row
return from_mapping(mapping, move_to_meta=move_to_meta, cosmology=cosmology)
cosmology/io/mapping.py:114: in from_mapping
return cosmology(*ba.args, **ba.kwargs)
cosmology/flrw/wpwazpcdm.py:141: in __init__
self.zp = zp
cosmology/parameter.py:138: in __set__
value = self.validate(cosmology, copy.deepcopy(value))
cosmology/parameter.py:186: in validate
return self.fvalidate(cosmology, self, value)
cosmology/parameter.py:348: in _validate_with_unit
value = u.Quantity(value, param.unit)
units/quantity.py:445: in __new__
value = value.to(unit)
units/quantity.py:935: in to
value = self._to_value(unit, equivalencies)
units/quantity.py:888: in _to_value
return self.unit.to(
units/core.py:1195: in to
return self._get_converter(Unit(other), equivalencies)(value)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = UnrecognizedUnit(redshift), other = Unit("redshift"), equivalencies = []
def _get_converter(self, other, equivalencies=None):
self._normalize_equivalencies(equivalencies)
> raise ValueError(
f"The unit {self.name!r} is unrecognized. It can not be converted "
"to other units."
)
E ValueError: The unit 'redshift' is unrecognized. It can not be converted to other units.
units/core.py:2015: ValueError
------------------------------------------------------------------------------------------- Captured stdout call -------------------------------------------------------------------------------------------
cosmology name H0 Om0 Ode0 ... Neff m_nu[0] m_nu[1] m_nu[2] Ob0
km / (Mpc s) ... eV eV eV
--------- ------- ------------ ---- ---- ... ---- ------- ------- ------- ----
wpwaCDM ABCMeta 70.0 0.27 0.73 ... 3.04 0.0 0.0 0.0 0.03FAILED cosmology/flrw/tests/test_wpwazpcdm.py::TestwpwaCDM::test_readwrite_mrt_instance - ValueError: The unit 'redshift' is unrecognized. It can not be converted to other units.
FAILED cosmology/flrw/tests/test_wpwazpcdm.py::TestwpwaCDM::test_readwrite_mrt_subclass_partial_info - ValueError: The unit 'redshift' is unrecognized. It can not be converted to other units.
FAILED cosmology/flrw/tests/test_wpwazpcdm.py::TestwpwaCDM::test_readwrite_complete_info[ascii.mrt-False-True] - ValueError: The unit 'redshift' is unrecognized. It can not be converted to other units.
FAILED cosmology/flrw/tests/test_wpwazpcdm.py::TestwpwaCDM::test_readwrite_from_subclass_complete_info[ascii.mrt-False-True] - ValueError: The unit 'redshift' is unrecognized. It can not be converted to other units.
FAILED cosmology/flrw/tests/test_wpwazpcdm.py::TestFlatwpwaCDM::test_readwrite_mrt_instance - ValueError: The unit 'redshift' is unrecognized. It can not be converted to other units.
FAILED cosmology/flrw/tests/test_wpwazpcdm.py::TestFlatwpwaCDM::test_readwrite_mrt_subclass_partial_info - ValueError: The unit 'redshift' is unrecognized. It can not be converted to other units.
FAILED cosmology/flrw/tests/test_wpwazpcdm.py::TestFlatwpwaCDM::test_readwrite_complete_info[ascii.mrt-False-True] - ValueError: The unit 'redshift' is unrecognized. It can not be converted to other units.
FAILED cosmology/flrw/tests/test_wpwazpcdm.py::TestFlatwpwaCDM::test_readwrite_from_subclass_complete_info[ascii.mrt-False-True] - ValueError: The unit 'redshift' is unrecognized. It can not be converted to other units. |
Member
Contributor
Author
|
@nstarman Thanks for the clarification. I was able listify the # Replace the m_nu column with three columns with names 'm_nu_[i]'
if "m_nu" in table.colnames:
m_nu = table_main["m_nu"]
table.remove_column("m_nu")
m_nu = np.atleast_1d(m_nu) # Ensure m_nu is an array
if m_nu.shape == (1,):
table.add_column(m_nu[0], name="m_nu[0]", index=-2) # Add a single column
else:
# Create multiple columns with appropriate names
cols, names = tuple(zip(*((m, f"m_nu[{i}]") for i, m in enumerate(m_nu[0]))))
table.add_columns(cols, names=names, indexes=(-4, -3, -2))I ran tests on my local machine and they passed successfully. |
Member
Great!
Always! Numpy is a runtime dependency for Astropy
|
Contributor
Author
@nstarman Do have an idea on how can I tackle this |
Changed format from "mrt" to "ascii.mrt" for read_mrt() and write_mrt() methods
Signed-off-by: Nathaniel Starkman <[email protected]>
This commit fixes few tests and changes table formatting
Mubin17
force-pushed
the
Mubin17/Issue#14871
branch
from
June 16, 2023 03:22
52dea54 to
aa5cfba
Compare
Signed-off-by: Nathaniel Starkman <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]>
Member
|
Ok. I found a workaround while diagnosing the issue! |
Contributor
|
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. |
Contributor
|
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. |
1 of 2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request is to address #14871
Fixes #14871
Registered the read_mrt() and write_mrt() methods with the Cosmology class, enabling the import and export of cosmological data to and from MRT files.
Writing Example:
Output:
Reading Example
Output: