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

Model presentation #62

Open
wants to merge 9 commits into
base: wd-v1.0
Choose a base branch
from
Open

Conversation

lmichel
Copy link
Collaborator

@lmichel lmichel commented Dec 5, 2024

  • remove 'abstract dimensional grouping' for errors
  • set errors (DataType) as attributes instead of composition
  • add a super class for Photometric properties

Copy link
Collaborator

@loumir loumir left a comment

Choose a reason for hiding this comment

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

commit photometric super class: class diagram :
I suggest that the relation between PhotometricProperty ( super class) and PhotCal should be maintained .
Photcal does not apply only to Brightness and should also apply to the Color class or other derived classes of PhotometryProperty.

@lmichel
Copy link
Collaborator Author

lmichel commented Dec 5, 2024

Agree with Mireille, I'll restore the PHOTCal link for colors

Copy link
Collaborator

@mcdittmar mcdittmar left a comment

Choose a reason for hiding this comment

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

Some details I'm noticing while going through the changes..
I have other comments from the previous version which I will submit as issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how this file relates to desc.mango.vo-dml.xml, but:

  • neither appears to have the text from desc.error.PropertyError.distribution.txt in them
  • other descriptions do not match (eg: Ellipse here does not have the new text seen in the other).

This is not enforced by the model. The definition of the ellipse attribute is imported from \texttt{coords:Ellipse}.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. I think the definitions match only for the standard a celestial coordinate space (see comments on issue Error types and relation to Measurement model Uncertainty #60.
  2. Ellipse is in Measurements, so is meas:Ellipse

@@ -1146,6 +1161,20 @@ As long as the vocabulary is not set, the possible values of this attribute are
<maxOccurs>1</maxOccurs>
</multiplicity>
</attribute>
<attribute>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm guessing that this metadata hasn't been needed in practice for the use cases, but added after re-reading the confidenceLevel discussion from Meas RFC1. ?? If this is associated with the confidenceLevel, they should be encapsulated in an object/datatype.

</attribute>
<attribute>
<vodml-id>EpochPositionErrors.properMotion</vodml-id>
<name>properMotion</name>
<description> Position error; can be an ellipse, a correlation matrix or a covariance matrix.</description>
Copy link
Collaborator

Choose a reason for hiding this comment

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

properMotion and position are both described with the same text as "Position error", is this intentional?

<name>properMotion</name>
</attribute>
<attribute>
<vodml-id>EpochPositionErrors.Position</vodml-id>
Copy link
Collaborator

Choose a reason for hiding this comment

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

'Position' should be lowercase

@@ -904,58 +890,62 @@ As long as the vocabulary is not set, the possible values of this attribute are
<vodml-id>EpochPositionErrors</vodml-id>
<name>EpochPositionErrors</name>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The migration from composition to attribute includes a change to the multiplicity of the different errors. This now requires errors for all properties where before they were not. Is that intentional?

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

Successfully merging this pull request may close these issues.

4 participants