Feat 363 analysis step #371

Sun-flow · 2023-06-13T16:02:28Z

No description provided.

saskiad · 2023-06-13T18:01:03Z

src/aind_data_schema/datamanipulation.py

+
+    ## These could be subtypes of analysis/processing
+
+class ProcessType(Enum):


how is this different from the ProcessType in processing.py?

sorry, ProcessName

processing.py currently does not have a ProcessType class, in my implementation at least. ProcessName would have values that should go here, most likely. I just had some examples in so that we could talk about the general structure first.

saskiad · 2023-06-13T18:05:16Z

src/aind_data_schema/datamanipulation.py

+
+
+
+class DataTouchInfo:


need doc strings for this class

src/aind_data_schema/datamanipulation.py

saskiad · 2023-06-13T18:05:16Z

src/aind_data_schema/datamanipulation.py

+
+    ## These could be subtypes of analysis/processing
+
+class ProcessType(Enum):


sorry, ProcessName

src/aind_data_schema/datamanipulation.py

saskiad · 2023-06-13T18:12:47Z

src/aind_data_schema/datamanipulation.py

+
+
+class DataTouchInfo:
+    touch_type: TouchType = Field(..., title="Manipulation type")


Is there a reason the title isn't the same as the name itself? Or vice versa?

i didnt know if using touch as a forward facing field would make sense to users? even though it makes senseo nthe backend. I can change one or the other to match.

saskiad · 2023-06-13T18:12:54Z

src/aind_data_schema/datamanipulation.py

+    notes: Optional[str] = None
+
+
+class CodeManipulation:


need docstring

This is a time when code is used? is that the idea? So not so much code manipulation, but data manipulation? Why is this separate from data touch? Oh because there can be data touches that don't use code... eg manual annotations. ... okay. I'm still not sure if this needs to be its own class or not... either it's a class that is optional or these are optional fields in the data touch class...

Sometimes all of these classes are going to be necessary, but I think dividing them up based on topic is useful for organization, and lets us see exactly what we are using. Additionally if more fields need to be added, there's some clear organization for where they go based on what they are related to.

saskiad · 2023-06-13T18:15:48Z

src/aind_data_schema/datamanipulation.py

+
+
+
+class DataTouchInfo:


Does this need the code manipulation class? (or code information?)

DataTouch is imported by AnalysisStep and ProcessStep. There are two structure options here:

DataTouch (overarching "Step" class):

class for basic info

either process/analysis tag

class for relevent step (analysis/processing)

class for codemanipulation info

but it seems you prefer the separation of Analysis/processing, so alternative looks like:

Process:

basic info about process (name/desc)

class for DataTouchInfo

class for codemanipulation info

Analysis:

basic info about analysis (name/desc)

class for DataTouchInfo

class for codemanipulation info

And some file tracking DataManipulations (we should maybe just change to this name,,,, since DataTouch is so weird...) would be a series of nodes (Process + Analysis Steps) tracking the history.

Sorry, I realize I got confused - too many different things swimming in my head at once. We want processing and analysis together - but I think then I missed something. Because the DataTouch is still the step, and we need something that has the list of steps, for both processing and analysis.
(and apologies if you jsut removed it in response to this)

let me make a figma doc detailing this organization, and I can show it to you later in the week

saskiad · 2023-06-13T18:16:38Z

src/aind_data_schema/datamanipulation.py

+    analysis_name: str = Field(..., description="Name of the analysis method used", title="Analysis name")
+    analysis_type: AnalysisType = Field(..., description="Type of analysis performed on dataset", title="Analysis type")
+    touch_info: DataTouchInfo = Field(..., description="General information regarding the data manipulation performed")
+    analysis_code: CodeManipulation = Field(..., description="Info regarding code used to manipulate data")


yeah, I think I prefer this to be in the data touch class.

im not sure I am following this comment

the analysis_code - I think this needs to live in the DataTouch class because it's part of how the data was used.

src/aind_data_schema/datamanipulation.py

saskiad · 2023-06-13T18:21:04Z

src/aind_data_schema/datamanipulation.py

+    processing_type: ProcessType = Field(..., description="Type of processing performed on dataset", title="Processing type")
+    touch_info: DataTouchInfo = Field(..., description="General information regarding the data manipulation performed")
+    analysis_code: CodeManipulation = Field(..., description="Info regarding code used to manipulate data")
+


do we need an Analysis model that creates a list of analysis steps?

ah! youve addressed my prior question, lol. It can easily, i would like to discuss briefly what it is that this model is doing first.

sorry - I think I confused myself. Let me take another look.

saskiad · 2023-06-13T18:21:26Z

src/aind_data_schema/processing.py

@@ -10,6 +10,7 @@

 from aind_data_schema.base import AindCoreModel, AindModel
 from aind_data_schema.imaging.tile import Tile
+from aind_data_schema.datamanipulation import DataTouchInfo


I'm a little confused - are we keeping this schema? How is this different from the processing in the datamanipulation file?

relic of an earlier edit, ideally we could drop the processing.py file after this update, though we could also have two separate files for analysis + processing if we wanted

saskiad · 2023-06-13T18:24:01Z

src/aind_data_schema/datamanipulation.py

+    """Description of a single processing step"""
+
+    analysis_name: str = Field(..., description="Name of the analysis method used", title="Analysis name")
+    analysis_type: AnalysisType = Field(..., description="Type of analysis performed on dataset", title="Analysis type")


I'm curious if we need an AnalysisType enum - or rather if we can even create it. I think this has to be a description that the scientist provides. Which is sad because they will probably not be good descriptions.

thats why I thought analysis type might be useful? at least to have some overarching classes -- that they could then provide further description for

but we need to have overarching classes of analysis, and I'm not sure we have them...

If that's the case then that's fine, we won't need an enum. It will be easy to remove in favor of a string.

saskiad · 2023-10-25T20:50:03Z

@Sun-flow can we delete this PR? We already closed this issue, correct? (I might be confusing things so correct me if I'm wrong)

saskiad · 2024-01-02T19:45:36Z

@Sun-flow can you confirm that this PR should be closed and deleted?

Sun-flow added 8 commits June 5, 2023 17:35

Initial Commit

e2aeb1f

Applying DataTouched object

00bb47f

Update processing.py

96aa4d6

Subdividing info

499f904

Update datamanipulation.py

1855ca5

Update datamanipulation.py

6f8a2ad

Update processing.py

75df835

Update datamanipulation.py

e09173f

Sun-flow added the enhancement New feature or request label Jun 13, 2023

Sun-flow requested a review from saskiad June 13, 2023 16:02

Sun-flow self-assigned this Jun 13, 2023

saskiad reviewed Jun 13, 2023

View reviewed changes

src/aind_data_schema/datamanipulation.py Outdated

class DataTouchInfo:

Copy link

Collaborator

saskiad Jun 13, 2023

Choose a reason for hiding this comment

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

need doc strings for this class

saskiad reviewed Jun 13, 2023

View reviewed changes

src/aind_data_schema/datamanipulation.py Outdated Show resolved Hide resolved

saskiad reviewed Jun 13, 2023

View reviewed changes

src/aind_data_schema/datamanipulation.py Outdated Show resolved Hide resolved

saskiad reviewed Jun 13, 2023

View reviewed changes

src/aind_data_schema/datamanipulation.py Outdated Show resolved Hide resolved

saskiad reviewed Jun 13, 2023

View reviewed changes

src/aind_data_schema/datamanipulation.py Outdated Show resolved Hide resolved

saskiad reviewed Jun 13, 2023

View reviewed changes

Sun-flow added 5 commits June 13, 2023 15:34

Update datamanipulation.py

9febc14

Update datamanipulation.py

5375a73

Update datamanipulation.py

219a15b

Update datamanipulation.py

eac58fb

Update processing.py

0f4aa43

second implementation

cfb8054

saskiad closed this Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat 363 analysis step #371

Feat 363 analysis step #371

Sun-flow commented Jun 13, 2023

saskiad Jun 13, 2023

saskiad Jun 13, 2023

saskiad Jun 13, 2023

Sun-flow Jun 13, 2023

saskiad Jun 13, 2023

saskiad Jun 13, 2023

saskiad Jun 13, 2023

Sun-flow Jun 13, 2023

saskiad Jun 13, 2023

saskiad Jun 13, 2023

Sun-flow Jun 20, 2023

saskiad Jun 13, 2023

Sun-flow Jun 13, 2023

saskiad Jun 13, 2023

Sun-flow Jun 20, 2023

saskiad Jun 13, 2023

Sun-flow Jun 13, 2023

saskiad Jun 13, 2023

saskiad Jun 13, 2023

Sun-flow Jun 13, 2023

saskiad Jun 13, 2023

saskiad Jun 13, 2023

Sun-flow Jun 13, 2023

saskiad Jun 13, 2023

Sun-flow Jun 13, 2023

saskiad Jun 13, 2023

Sun-flow Jun 20, 2023

saskiad commented Oct 25, 2023

saskiad commented Jan 2, 2024


		## These could be subtypes of analysis/processing

		class ProcessType(Enum):



		class DataTouchInfo:
		touch_type: TouchType = Field(..., title="Manipulation type")

Feat 363 analysis step #371

Feat 363 analysis step #371

Conversation

Sun-flow commented Jun 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saskiad commented Oct 25, 2023

saskiad commented Jan 2, 2024