Skip to content
This repository has been archived by the owner on Mar 15, 2023. It is now read-only.

Device and ImagingPlane should not be top properties of the Ophys schema. #539

Closed
h-mayorquin opened this issue Jun 1, 2022 · 10 comments
Closed
Assignees
Labels
discussion Let's talk about it organization May not reduce code or improve coverage, but enhances structure schema

Comments

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Jun 1, 2022

While working in #537 and #538 I was thinking about the following:

Context

Right now, we have objects like Device and ImagingPlane at the top of the Ophys schema, These objects though, usually are bound to other objects when we write them into the nwb_file (e.g. ImagingPlane for example is a property of TwoPhotonSeries in the imaging interface and ends up there):

metadata_schema["properties"]["Ophys"]["properties"] = dict(
Device=dict(type="array", minItems=1, items={"$ref": "#/properties/Ophys/properties/definitions/Device"}),
ImagingPlane=dict(
type="array", minItems=1, items={"$ref": "#/properties/Ophys/properties/definitions/ImagingPlane"}
),
TwoPhotonSeries=dict(
type="array", minItems=1, items={"$ref": "#/properties/Ophys/properties/definitions/TwoPhotonSeries"}
),
)

There is a similar situation with Device and ImagingPlane at the top level of Ophys in the schema for the BaseSegmentationExtractorInterface:

metadata_schema["properties"]["Ophys"]["properties"] = dict(
Device=dict(type="array", minItems=1, items=get_schema_from_hdmf_class(Device)),
)
metadata_schema["properties"]["Ophys"]["properties"].update(
Fluorescence=get_schema_from_hdmf_class(Fluorescence),
ImageSegmentation=get_schema_from_hdmf_class(ImageSegmentation),
ImagingPlane=get_schema_from_hdmf_class(ImagingPlane),
TwoPhotonSeries=get_schema_from_hdmf_class(TwoPhotonSeries),
)
metadata_schema["properties"]["Ophys"]["required"] = ["Device", "Fluorescence", "ImageSegmentation"]

Proposal

My question is, would it not be better to have this metadata nested to the level that they actually belong to?

For example, for the BaseImagingExtractorInterface would it not make more sense to have ImagingPlane as a property of the TwoPhotonSeries at the metadata level? That is the level that it occupies in the actual schema and we eventually send the object there with the add_two_photon_series function.

The only reason that I can see to keep these objects like Device and ImagingPlane at the top level is that maybe is easier to link themacross modalities (for example if the segmentation extractor and the imaging extractor share an ImagingPlane or a Device). In that case, I think that we can simple link the objects with the name if the device already exists in the nwb_file.

Similar concenrs applie for Device in both classess and to TwoPhotonSeries in the BaseSegmentationExtractorInterface (where it should be nested in ImageSegmentation). I feel that setting them at the right (nested) level avoids a type of ambiguity when, for example, more than one Device or ImagingPlane is used and it also allows for better control of the properties of the schema.

Maybe I am missing something?

TL;DR We should have the metadata properly nested in the place that they occupy in the schema and link by name if we need to.

@h-mayorquin h-mayorquin added discussion Let's talk about it organization May not reduce code or improve coverage, but enhances structure schema labels Jun 1, 2022
@h-mayorquin h-mayorquin self-assigned this Jun 1, 2022
@h-mayorquin h-mayorquin changed the title Device and ImagingPlane should not be top properties of the Ophy schema. Device and ImagingPlane should not be top properties of the Ophys schema. Jun 1, 2022
@bendichter
Copy link
Collaborator

Actually both of these objects are example of links. They are stored in /general/optophysiology/ and linked to by other objects like TwoPhotonSeries. If you look in the TwoPhotonSeries spec you will see that Device and ImagingPlane are in fact both Links. This allows the same Device or the same ImagingPlane to be linked to in another TwoPhotonSeries. So it makes sense that they are not actually defined within the TwoPhotonSeries. A counter-example is OpticalChannel, which is not a link and is defined within ImagingPlane

@CodyCBakerPhD
Copy link
Member

This structure is also designed to mimic the metadata and object links on the ecephys side (EcephysDevice -> ElectrodeGroup -> Electrodes -> ElectricalSeries)

@CodyCBakerPhD
Copy link
Member

I had closed this issue since I presumed that after 3 weeks of non-response to Bens original explanation, this had been explained.

Apparently @h-mayorquin was not satisfied, and was upset that this was closed (though anyone should still be able to comment on issues after their closure, as well as re-open any issue they made that was then closed - let me know if this is not behaving correctly).

@h-mayorquin Can you please clarify what you disagree with, propose an alternative solution that is better, and emphasize why this is a current item of priority interest?

Sorry for the confusion this caused and presumptions on my part.

@CodyCBakerPhD CodyCBakerPhD reopened this Jun 20, 2022
@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Jun 20, 2022

Thanks for opening it again @CodyCBakerPhD.

For the record:

  • I could not re-open the issue myself (there is no obvious button for that and when I read about it, the documentation pointed out that is a matter of access privileges). If I could do it before and I did not know how, fine enough, mea culpa.
  • If this is not true and you can point out to me how can I re open issues myself this problem should be solved.
  • I am not upset that you closed my issue (you could have mistaken my silence as disengagement and that's fine), I asked you personally to re-open it as I had not answered and for some reason your first reaction was to refuse my request. I was upset about that. I don't think you should have the power to refuse such request and I still feel like that.
  • Your timeline does not match. There was not three weeks between response and closing.

I am fine with having the policy of closing issues that are stale for sometime. I was busy and could not follow up on this as quickly as I would like. I would also like to say that we have plenty of issues that are stale for far longer than 3 weeks so maybe we should write this somewhere or do use a both that gives a fair warning to avoid these confusions in the future.

@CodyCBakerPhD
Copy link
Member

I could not re-open the issue myself (there is no obvious button for that and when I read about it, the documentation pointed out that is a matter of access privileges). If I could do it before and I did not know how, fine enough, mea culpa.
If this is not true and you can point out to me how can I re open issues myself this problem should be solved.

Hmm... I'll look into that, then.

I asked you personally to re-open it as I had not answered and for some reason your first reaction was to refuse my request. I was upset about that. I don't think you should have the power to refuse such request and I still feel like that.

You DM'd me first on the Saturday evening of a holiday weekend (Father's day + Juneteenth, including the federal holiday extension of that to today, the 20th), then began accusing me of ignoring you before I finally had time to respond for the first time today (again, technically a holiday for our federally-related project). While it is a milestone of the U24 grant to have a certain response time on freshly posted issues (and I do everything in my power to uphold that), that does not apply to private DM's on Slack. Please be more patient for this kind of thing in the future.

Your timeline does not match. There was not three weeks between response and closing.

Based on #539 (comment), 2 weeks at time of closure, 3 weeks by time of request to re-open. Rounding to nearest week +/- few days.

@CodyCBakerPhD
Copy link
Member

Hmm... I'll look into that, then.

Yeah, it's nowhere to be found in the admin controls. Looking around led to

Old stackoverflow question: https://stackoverflow.com/questions/21333654/how-to-re-open-an-issue-in-github
The old GitHub feature requests: isaacs/github#583

Seems like just one of those things where the GitHub user community is too divided on the issue to let it move forward. Personally I don't see the problem in making it configurable, though.

@h-mayorquin
Copy link
Collaborator Author

I asked you personally to re-open it as I had not answered and for some reason your first reaction was to refuse my request. I was upset about that. I don't think you should have the power to refuse such request and I still feel like that.

You DM'd me first on the Saturday evening of a holiday weekend (Father's day + Juneteenth, including the federal holiday extension of that to today, the 20th), then began accusing me of ignoring you before I finally had time to respond for the first time today (again, technically a holiday for our federally-related project). While it is a milestone of the U24 grant to have a certain response time on freshly posted issues (and I do everything in my power to uphold that), that does not apply to private DM's on Slack. Please be more patient for this kind of thing in the future.

That's fine, if you felt that I was rushing you to answer I apologize for that. I just wanted to clarify that my issue was your first refusal to re-open the issue and not (as you mentioned above) that you closed the request. Telling me that you could not do it right now because you were busy or just ignoring me until you were free would be fine in my book. But refusing to do it, with that I disagree. That's it. If you were not refusing to re-open it then that's a misunderstanding on my side and I am fine with moving forward and leaving it like that.

Your timeline does not match. There was not three weeks between response and closing.

Based on #539 (comment), 2 weeks at time of closure, 3 weeks by time of request to re-open. Rounding to nearest week +/- few days.

I am fine with that clarification, but if you want to implement a strict policy of closing issues you should clarify between these differring interpretations (time since last activity vs time from opening vs time from last activity by the user who open).

@CodyCBakerPhD
Copy link
Member

If you were not refusing to re-open it then that's a misunderstanding on my side and I am fine with moving forward and leaving it like that.

Sounds like a simple misunderstanding, then. I never 'refused' (a strong word you have now used 5 times to refer to this occasion) to do so, nor would I as long as your request was based on the issue not being resolved.

It might be helpful to do an explicit review of the communication:

Heberto, 5:37pm Saturday - Can you open this again? #539
I don't know why you close it, I would like to discuss it with Ben in our next meeting.

Heberto, 12:40 Monday - Why are you not answering this? It is already rude that you close an issue for another developer on the project without previous warnings and now you don't answer here?

Cody, 12:47 Monday - Response times can certainly be variable over a holiday weekend (check US holidays). You can always discuss things in meetings regardless of if they are open or closed. As for the issue itself, you asked a question and it was answered definitively by both of us.

The good thing is we can treat this misunderstanding as a test run of user outreach/response; what could I have improved upon in my communication with you to avoid this misunderstanding in the future (and more importantly, with users)?

The way I see it, my initial response clarified why I had closed it, and pointed out that the original reasoning for re-opening was not relevant to the open/close status - as soon as you stated that you did not consider the matter closed (and as soon as I had time) the issue was indeed re-opened.

I am fine with that clarification, but if you want to implement a strict policy of closing issues you should clarify between these differring interpretations (time since last activity vs time from opening vs time from last activity by the user who open).

It's not a strict policy, it's just whenever I get around to combing through open issues (could be once a week, could be once a month). I only set reminders for important things like deprecations.

An issue bot might be the best way to go for automating that process, could you look into that sometime?

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Jun 20, 2022

The source of the misunderstanding:

Cody, 12:47 Monday - Response times can certainly be variable over a holiday weekend (check US holidays). You can always discuss things in meetings regardless of if they are open or closed. As for the issue itself, you asked a question and it was answered definitively by both of us.

It came from 1) you saying that (in bold) and 2) not opening the issue after that, 3) start another discussion about whether issues should in fact be closed or not. That I interpreted as you refusing to re-open. If you had said, "OK, I was busy, I will re-open it as soon as I have some time" that would have defused it.

I also would like to ask you to no to share our private conversation without my consent, it is not a big deal for me but I think is the wrong procedure and it leaves a bad precedent. I also think that you should not have started saying that I was upset above when you re-opened the issue here, sets a bad tone to re-start the discussion. I went to you privately the first time because I think this whole ordeal would have been better addressed that way. I honestly think we both would have looked better if we had kept it that way

@bendichter
Copy link
Collaborator

@h-mayorquin and I discussed this and agreed to close.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion Let's talk about it organization May not reduce code or improve coverage, but enhances structure schema
Projects
None yet
Development

No branches or pull requests

3 participants