MivotInstance update#747
Conversation
lmichel
commented
Apr 17, 2026
lmichel
commented
- FEATURE: Return None when accessing missing attributes, e.g. not mapped model field
- BUG FIX: Support unit strings with a DOT, e.g. ms.year-1
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #747 +/- ##
==========================================
- Coverage 79.80% 79.79% -0.01%
==========================================
Files 91 91
Lines 10297 10297
==========================================
- Hits 8218 8217 -1
- Misses 2079 2080 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
On Fri, Apr 17, 2026 at 09:07:55AM -0700, Laurent MICHEL wrote:
- BUG FIX: Support unit strings with a DOT, e.g. ms.year-1
Since I just spotted it: That's not a valid VOUnits string. As it
says in the short VOUnits summary: "Raising a unit to a power is
indicated only with the symbol ‘**’ (not a ‘^’ or by simply appending
an integer)". But you certainly shouldn't parse VOUnits yourself
anyway. There's a parser for them in astropy.
|
simplification of the readout code
|
Thank you for your comments.
I took this opportunity to simplify the code in question a bit and expand the tests. |
tomdonaldson
left a comment
tomdonaldson
left a comment
There was a problem hiding this comment.
Thanks @lmichel ,this looks good to me. I had one minor question, but will approve.
There is also a minor merge conflict. I can resolve that if it's helpful.
| mivot_object = MivotInstance(**mydict) | ||
| assert mivot_object.longitude.unit == "mas.year-1" | ||
| mydict["dmid"] = "a.b.c" | ||
| mivot_object = MivotInstance(**mydict) |
There was a problem hiding this comment.
Should there be an assert for this object or is it just ensuring that no exceptions are thrown on the instantiation?
There was a problem hiding this comment.
It just to check that no exceptions are thrown
|
Conflict in change log fixed |
|
|
||
| - Improve ``pyvo.mivot.viewer.%ivotInstance`` (API to allow users to access the mapped data | ||
| attributes directly as properties of the instance); [#747] | ||
| - Access to not exiting attribute, e.g. not mappped, returns ``None`` |
There was a problem hiding this comment.
| - Access to not exiting attribute, e.g. not mappped, returns ``None`` | |
| - Access to non-existent attribute, e.g. not mapped, returns ``None`` |
| - Improve ``pyvo.mivot.viewer.%ivotInstance`` (API to allow users to access the mapped data | ||
| attributes directly as properties of the instance); [#747] | ||
| - Access to not exiting attribute, e.g. not mappped, returns ``None`` | ||
| - Support DOTS in unit string, e.g. "mas.year-1" |
There was a problem hiding this comment.
Not obvious how this was achieved? By removing code?
There was a problem hiding this comment.
by implementing a __getattr__(self, item) method. Should the change log give this detail?
There was a problem hiding this comment.
nope, no need for these internal details in the changelog. If you want you can leave some comments in the source code; but I'm not sure how much clearer it would make things.
There was a problem hiding this comment.
There is a long comment telling what this method is for
5 participants
