eurec4a / flight-phase-separation

Collection of manually edited flight segments for all platforms participating in EUREC4A.
1 stars 6 forks source link

Updated conventions to reflect conversation in issue #2 #7

Closed RobertPincus closed 4 years ago

RobertPincus commented 4 years ago

This PR includes

@gdeboer2 @leifdenby Further refinements welcome.

gdeboer2 commented 4 years ago

I think that mutually exclusive kinds would be great, but the primary items listed are not mutually exclusive (e.g. see my comment about racetracks, transits and circles). Mutually exclusive, in my opinion, would only be terms that describe the aircraft attitude (e.g. changing altitude or not and/or turning or not).

Gijs

On Aug 4, 2020, at 1:39 PM, Robert Pincus notifications@github.com wrote:

@RobertPincus commented on this pull request.

In README.md https://github.com/eurec4a/flight-phase-separation/pull/7#discussion_r465286441:

@@ -57,3 +57,7 @@ HALO uses the following sub-kinds of the calibration kind:

  • radar_calibration_wiggle
  • radar_calibration_tilted
  • baccardi_calibration
  • +## Conventions +Flight segmentation is designed to be flexible and unstructured, but we propose that data providers follow the convention that +a time or time window should not belong to more than one segment of the same type @gdeboer2 https://github.com/gdeboer2 We should specify same kind - that is, a time can't be part of one circle and another circle too.

We had talked about adding a list of mutually-exclusive kinds. I didn't put this in the description because we don't have plans but I could mention it if you thought that was helpful.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eurec4a/flight-phase-separation/pull/7#discussion_r465286441, or unsubscribe https://github.com/notifications/unsubscribe-auth/AO3B45QBYQ22Y6LMTMYQKCTR7BPXVANCNFSM4PUQOURA.

d70-t commented 4 years ago

I like that mutual exclusion is not yet part of this PR. I think it is really hard to do that right in the end (apart from the attitude, as you said). When we add a mutual exclusion list, I'd only add things to it once we find an actual need for it.

RobertPincus commented 4 years ago

@gdeboer2 @d70-t I've revised the text to incorporate these suggestions. Some notes:

If you are both happy either of you might merge the PR.

d70-t commented 4 years ago

I am fine with it. If you, @gdeboer2 are as well, I'd merge it.

leifdenby commented 4 years ago

Looks good to me too. The only thing I would suggest is building a common vocabulary for the segment types (stored in a python module) and writing a test that ensures that only those are used. This will avoid duplication and we can add a note in the module to keep the README up-to-date. Checking the vocabulary could be added to scripts/verify.py (although personally I'd put these kind of tests in tests/ and run them with pytest). What do you think @d70-t ?

d70-t commented 4 years ago

Yes, @leifdenby I also had a look at your concept for testing the intake catalogs with pytest and I think I like it. The original idea of the checks which are used for the verification step has been that these warnings will show up in the generated HTML report alongside some plots which illustrate the segments. I really like to keep that functionality and I did not yet think through how we could keep that functionality while migrating to a pytest based approach.

I could have a look into creating such a controlled vocabulary file, but I'd propose to track that in #4 as well.

RobertPincus commented 4 years ago

@leifdenby @d70-t My sense from the conversation is that we don't want a controlled vocabulary, at least not yet. (See also #4). People may want to use the YAML files in many way. In fact with the ATR you can already see that users have very different needs - some want very finely delineated segments, others only broad distinctions. The benefit to a controlled vocabulary would be to help users be consistent between and among platforms. I'd suggest waiting for a use case before imposing the burden.

d70-t commented 4 years ago

I think the burden would be quite low, if people only would have to add their newly invented kinds into one common (YMAL?) file (maybe including a little description) and include that to the pull request. Anyways, that will not be handled in this PR and I don't see other objections, so I'll merge it.

gdeboer2 commented 4 years ago

Apologies for the delayed response. I’m having a hard time keeping up with this discussion. Please move ahead without my input.
Unlikely to get back to this until after next week at the earliest, and haven’t yet determined how exactly this will align with the RAAVEN dataset.

On Aug 12, 2020, at 9:09 AM, Tobias Kölling notifications@github.com wrote:

I think the burden would be quite low, if people only would have to add their newly invented kinds into one common (YMAL?) file (maybe including a little description) and include that to the pull request. Anyways, that will not be handled in this PR and I don't see other objections, so I'll merge it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eurec4a/flight-phase-separation/pull/7#issuecomment-672932186, or unsubscribe https://github.com/notifications/unsubscribe-auth/AO3B45VAITBVFF7L2QCHO4TSAKWEHANCNFSM4PUQOURA.