fix: fix tdim issue - #46
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 46.35% 46.68% +0.33%
==========================================
Files 17 17
Lines 2550 2551 +1
==========================================
+ Hits 1182 1191 +9
+ Misses 1368 1360 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
11c5a1b to
cef8eb2
Compare
There was a problem hiding this comment.
- Remove prepended underscore, i.e.,
_parse_tdim()->parse_tdim(), - The argument string should not require the
strip()function. If it does, then there is a problem with the initial string parsing function or regex, - Suggest parsing the string using a Regex to ensure that the
Tuplevalues are integers, instead ofeval()or using areplace()to insert the missing comma, - The function should return a
Tupleornothing, wherenothingmeans thatTDIMjis either missing or an empty string, and - The function should be more concise, i.e., no more that 1 or 2 lines of code.
Note that the TDIMj Tuple also needs to be reconciled with the array size and element type. For example, if the element type is a string then, the TDIMj should return a Tuple of one fewer dimensions, because the first dimension gives the size of the string. See the FITS standards document for details.
|
Hi @barrettp, parse_tdim now uses regex, returns nothing for missing/empty values, and handles FITS header padding without strip. Tests also cover one-dimensional and multi-dimensional values plus invalid input. Thank you! |
Closes #44
Fix one-dimensional FITS TDIM values such as TDIM=(3). These were parsed as an integer, but BinaryField expects dimensions as a tuple or nothing. The new helper normalizes them to (3,) across all binary-table construction paths and adds tests for the changes.