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

Problem with capital letters in column names #79

Closed
folsomcp opened this issue Sep 28, 2024 · 4 comments
Closed

Problem with capital letters in column names #79

folsomcp opened this issue Sep 28, 2024 · 4 comments

Comments

@folsomcp
Copy link

I ran into a problem using capital letters in table column names with the SoniSeries class. Specifically, if I use capital letters in the column name in the Astropy Table, and use the same capital letters in the value_col or time_col fields SoniSeries I get an error. If I use all lower case the it works fine.

So this works as expected

from astropy.table import Table
from astronify.series import SoniSeries
data_table = Table({"time":[0, 1, 2, 3, 4, 5, 9, 10, 11, 12],
                    "fluxu": [0.3, 0.4, 0.3, 0.5, 0.5, 0.4, 0.3, 0.2, 0.3, 0.1]})
data_soni = SoniSeries(data_table, time_col='time', val_col='fluxu')

But this version

data_table = Table({"time":[0, 1, 2, 3, 4, 5, 9, 10, 11, 12],
                    "fluxU": [0.3, 0.4, 0.3, 0.5, 0.5, 0.4, 0.3, 0.2, 0.3, 0.1]})
data_soni = SoniSeries(data_table, time_col='time', val_col='fluxU')

gives an error:

AttributeError: Input Table must contain a value column 'fluxU'

I took a quick look at the astronify/series/series.py file. In the SoniSeries class data function, around lines 151 - 152, there is a bit of code that seems to convert the data_table columns to all lower case. But I didn't see any corresponding code to convert the time_col and val_col values to all lowercase. Perhaps that miss-match is causing my issue!

@scfleming
Copy link
Collaborator

You are most likely correct, I'll see about fixing that bug in an update!

@scfleming
Copy link
Collaborator

Hi @folsomcp, I am unable to reproduce your error using the most recent version of the software I am working on here. It's possible you were using an older or modified version of the code? Very shortly we will be releasing a new version, once we do (or if you would like to try a fresh install), could you let me know if you still have this error? I tried both versions of your example and both are able to have a SoniSeries created and sound made.

@folsomcp
Copy link
Author

Hi @scfleming thanks for looking into this. The problem still exists on the main branch, but that hasn't been updated. It looks like py311 is your current development branch? I tried a fresh install of the py311 branch. But I ran into a problem that it requires Python version <3.12, and I am running Python 3.12.3, so the install failed. If you can relax the Python version requirement to allow newer versions I can give it another shot.

Looking at the code I suspect the bug might be solved. In the main branch it seems that the problem likely was in astronify/series/series.py in the SoniSeries class.

    def data(self, data_table):

        if not isinstance(data_table, Table):
            raise TypeError('Data must be an astropy.table.Table object.')

        for c in list(data_table.columns):
            data_table.rename_column(c, c.lower())

And my guess is the data_table.rename_column(c, c.lower()) was causing me trouble.

The py311 branch don't have those lines switching things to lower case (maybe that was removed when the code switched to using assert isinstance(...) ?). So it looks like this might work fine. Although that's speculation without testing!

@scfleming
Copy link
Collaborator

At the time I was having trouble getting it to install with Python 3.12, I think due to pyvo, but it's working now with the latest version of pyvo, so I am unpinning on the right! Going to be merging the (now not ideally named) py311 today into main and releasing, which is intended to also work with Python 3.12. Do let me know if you have any troubles though as a separate Issue! And thanks for all your help!

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

No branches or pull requests

2 participants