Skip to content

CDMS: fix MOLWT column starting position#3094

Closed
kmaitreys wants to merge 2 commits intoastropy:mainfrom
kmaitreys:main
Closed

CDMS: fix MOLWT column starting position#3094
kmaitreys wants to merge 2 commits intoastropy:mainfrom
kmaitreys:main

Conversation

@kmaitreys
Copy link

Now the molecule tag 100501 and others work. Earlier they were crashing with

ValueError: invalid literal for int() with base 10: '13-'

Because that - is part of the signed TAG.

@bsipocz bsipocz requested a review from keflavich September 18, 2024 17:58
@bsipocz bsipocz changed the title fix MOLWT column starting position CDMS: fix MOLWT column starting position Sep 18, 2024
@bsipocz bsipocz added this to the v0.4.8 milestone Sep 18, 2024
@bsipocz bsipocz added the cdms label Sep 18, 2024
@bsipocz
Copy link
Member

bsipocz commented Sep 18, 2024

RTD failure is unrelated and will be fixes outside this PR.

@keflavich
Copy link
Contributor

We'll need some tests for this error. I can confirm that 100501 does not work right now:

CDMS.query_lines(10*u.GHz, 100*u.GHz, molecule='100501', parse_name_locally=False)

I tested one other case, and it worked both before & after:

CDMS.query_lines(10*u.GHz, 100*u.GHz, molecule='HCCCN', parse_name_locally=True)

so I'm going to approve this, but on my to-do list is to systematically test all molecules for failures like this.

@bsipocz
Copy link
Member

bsipocz commented Sep 19, 2024

In addition to putting it on the to-do, please do open an issue for it here, too.

@keflavich keflavich mentioned this pull request Sep 19, 2024
2 tasks
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failures are related

@kmaitreys
Copy link
Author

Yeah, I'm sorry, the last commit I pushed actually broke a bunch of other molecules. So far from my testing/debugging, it looks like just changing the column numbers will not help, because the format itself is not consistent for all molecules. It needs a bit larger refactor in some parts.

I'm still up for fixing this, but couldn't due to my own work. If it's possible, I would like this to be kept open for a few weeks more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants