Initial version of BaseEnhancedTable and PhotometryData added

[JuanCab]

Fixes #114 and fixes #115.

I added an initial version of the BaseEnhancedTable and PhotometryData classes to the core.py file and added extensive tests to the tests/test_core.py.

Please note while BaseEnhancedTable does force certain attributes to exist, the PhotometryData did NOT add any required attributes. Its easy enough to add, I just wasn't sure what you wanted to require as being there.

[mwcraig]

I'm sure I'll have more to say -- I'm going to look over the metaprogramming section of fluent python to see if there is a different approach to take instead of subclassing.

[mwcraig]

Don't think we need this. The new stuff has documentation but that should be the norm going forward.

[JuanCab]

Noted.

[JuanCab]

Well let me know. I looked at @dataclasses but subclassing data classes looked... painful and it forced an order to parameters I was unhappy with.

[JuanCab]

I went through your comments and tried to address most of them.

[JuanCab]

'is not' is checking for identity being the same, e.g. they are the SAME OBJECT, != checks for equality. I suspect it fails with 1/adu because that is a constructed unit and so two different instances of it are considered two different objects. An equality check seems to be the way to go.

[JuanCab]

Also, I think we'll need to allow for the possibility that there is not a corresponding filter, eg 540_bp40.

Well, the way the code currently currently works, if the filter is not mentioned in the filter_map, the filter will simply not be changed. So the filter_map should only be for those filters that do map to standard AAVSO names.

[JuanCab]

I went through your comments and tried to address most of them.

[mwcraig]

I'm not 100% sure the discussion about types went off to, but

Can you confirm if you subsequently add new data to the text column that is longer than the initial allocation, the string length gets updated?

The answer seems to be complicated. If you start with an empty table the only way to populate it seems to be add_row or add_rows. If you add data that way, then the string column length gets changed automatically.

If the table already has data then what happens if you try to update a string column depend ons on how you do it.

If you create the table and add a row like this

from astropy.table import Table
t = Table(names=['decimal', 'text'], dtype=('float', 'str'))
t.add_row((10.0, "hello"))

then t["text"] is string of length 5.

Updating like this changes to length 7:

t["text"] = ["longest'}

but if you instead try this it truncates:

t["text"][:] = ["longest"]

I think the reason is that t["text"] = ... is really replacing the original column, not simply updating the values The second case updates the vlaues but also warns that they are being truncated.

[mwcraig]

Something else just occurred to me -- changing an attribute doesn't change the underlying table, so an attribute and its corresponding table column can get out of sync.

Not sure how to deal with this; one option might be to make properties instead of settable attributes.

To reproduce, run this, and note that at the end bet.number and bet.data["number"] are different:

import numpy as np
from astropy.table import Table

table_description = np.array([["number", float, None, "number"],
    ["ra", float, "degree", "ra"],
    ["dec", float, "degree", "dec"],
    ["id", int, None, "id"]
])

tab = Table(data=[[1], ["abc"], [1.0], [1.0], [5.1]], names=("id", "tex
    ...: t", "ra", "dec", "number"), units=(None, None, "degree", "degree", None
    ...: ))

bet = BaseEnhancedTable(table_description, data=tab)

bet.number

# Returns
# <Column name='number' dtype='float64' length=1>
# 5.1

bet.number = [6.1]
bet.number

# Returns
# [6.1]

bet.data["number"]

# Returns
# <Column name='number' dtype='float64' length=1>
# 5.1

[JuanCab]

Something else just occurred to me -- changing an attribute doesn't change the underlying table, so an attribute and its corresponding table column can get out of sync.

Not sure how to deal with this; one option might be to make properties instead of settable attributes.

To reproduce, run this, and note that at the end bet.number and bet.data["number"] are different:

import numpy as np
from astropy.table import Table

table_description = np.array([["number", float, None, "number"],
    ["ra", float, "degree", "ra"],
    ["dec", float, "degree", "dec"],
    ["id", int, None, "id"]
])

tab = Table(data=[[1], ["abc"], [1.0], [1.0], [5.1]], names=("id", "tex
    ...: t", "ra", "dec", "number"), units=(None, None, "degree", "degree", None
    ...: ))

bet = BaseEnhancedTable(table_description, data=tab)

bet.number

# Returns
# <Column name='number' dtype='float64' length=1>
# 5.1

bet.number = [6.1]
bet.number

# Returns
# [6.1]

bet.data["number"]

# Returns
# <Column name='number' dtype='float64' length=1>
# 5.1

This doesn’t surprise me, you can take any variable and replace its value with a new one. I just created an attribute that pointed to a astropy table column. Resetting that attribute to a list breaks that pointer.

[edited] As you suggested, we might be able to handle this by decorating it with @property and adding the corresponding getter/setter/deleter functions. The only trick is constructing functions with names based on the columns in a dynamic way. Essentially we would have to create template functions and then assign attributes to the functions, not sure if that will work.

[JuanCab]

I have uploaded a complete rewrite pair of data classes constructed based on feedback from Astropy folks at SciPy2023. This is what caused so much grumbling during the sprints, but it is done.

[JuanCab]

On the flight I hit on the solution for computing the ‘night’, so that’s taken care of as well.

[mwcraig]

Excellent progress! There are a couple suggestions inline for some changes. We should also talk briefly about how data gets added to this structure by stellarphot as it is doing photometry.

[mwcraig]

I don't think we need to be this restrictive. The only thing we really need is that table_description is accessible via keys. We may want to make sure this is a dictionary for us, so we could do something like this up where you currently set self._table_description = table_description (lines below are untested, meant to illustrate the idea)

try:
    self._table_description = {k: v for k, v in table_description.items()}
except AttributeError:
    raise TypeError(...)

[JuanCab]

I just discovered the required column checking I thought was being done is not, so still need to debug this.