-
Notifications
You must be signed in to change notification settings - Fork 0
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
App Submitted - swt-detection.v4.0 #124
Conversation
Looks like this version outputs the |
Hmmm, are you referring to the parameters? |
Not really... I'm referring to this part of metadata.py |
Ah, I was just looking at that. It is unclear to me what should be done with those now that we have the label set on the metadata. The MMIF output seems to trudge along just fine and does not seem to be impacted by those lines, at least, not as far as I can see. What does worry me a bit is that no parameters are added. This was probably always the case, and I guess it is okay because we use the defaults, but we may want to think about whether these defaults should be in the view metadata. |
Yeah, currently there's no mechanism to automatically validate the MMIF output based on the app metadata. Writing correct and precise metadata is mainly for human users at the moment.
Default values are added during parameter refinement process (https://github.com/clamsproject/clams-python/blob/7f8bb0ba9c955b12f1bf82a830ffb95e20ef0c51/clams/app/__init__.py#L168-L169) . We discussed adding the "refined" version of the parameter map to the output MMIF (clamsproject/clams-python#166), but later we decided not to do that (clamsproject/mmif#216). |
Okay, so what needs to be done to get this out? Just replace metadata.add_output(AnnotationTypes.TimeFrame, timeUnit='milliseconds', label='NEG') Or do metadata.add_output(AnnotationTypes.TimeFrame, timeUnit='milliseconds', labelset=['bars', 'slate', ...]) The latter is my preferred one as long as it is alowed. I thought the result of our previous discussions on parameters was to add them as handed in by the user. I see how that is ambiguous between using what is in the defaults and what is handed in by the user, with the later being the more prominent interpretation, so what we have now is fine. |
I also think, with the output syntax change, the latter makes lot more sense now. |
Righto, will do that. Which leads to another question. At this point, with the v4.0 tag already added on the main branch in the app, how do we proceed to make the changes? And will they be incorporated into 4.0? I guess the app does not get published until this pull request is merged. But the process is not clear to me. Aside from making changes in the metadata I would also like to include the fix for the label on the TimePoint. This may better be discussed in person or over zoom. |
So here's what I've been doing with other RA devs in the team. We just close a PR as "not-approved" when the submitted app has a problem, and the app dev can go back to the app repo and take the tag down and make necessary changes/commits. Once the app's ready, they simply re-tag a commit on their default branch (of the app repo), and the app will be re-submitted. |
Okay, thanks, that looks pretty straightforward. |
(no connected issue)
This PR registers the app swt-detection.v4.0 from https://github.com/clamsproject/app-swt-detection/tree/v4.0. Carefully review the metadata before merging. If something is wrong with the metadata, please close the PR and let the app developer know.
(submitted by @marcverhagen)