search

AI-multimodal / aimm-post-processing

This repository is depricated. We will be doing all postprocessing in `aimmdb.postprocessing`.
Other
0 stars 3 forks source link

Matt dev for demo #18

Open matthewcarbone opened 1 year ago

matthewcarbone commented 1 year ago

@zleung9 you good with all this?

zleung9 commented 1 year ago

Hi Matt,

Thank you for the comment. I am on a trip right now. I will review and get back to you later today.

Best, Zhu

Sent from my iPhone

On Aug 12, 2022, at 4:12 PM, Matthew Carbone @.***> wrote:

 @x94carbone requested your review on: #18 Matt dev for demo.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because your review was requested.

matthewcarbone commented 1 year ago

Btw, we will be moving everything to aimmdb soon, to consolidate the development process. For now let's get this merged then I'll update you!

zleung9 commented 1 year ago

Hi Matt @x94carbone , Your changes look very good to me. I have three comments:

  1. The metadata seem to keep the “post-processing” part and discards the original data information, such as sample, instrument, etc. which I think it important. It allows people to refer to the source at any stage of post-processing.
  2. The UnaryOperator call now accepts “client” and “node” as input. This works for the first operation in the pipeline. It seems that all the operations inherit from UnaryOperator, I think they need to accept the intermediate data format (dictionary of data frame and metadata, or a list of dictionaries in the “node” case)as input. My suggestion is to add an option such call_on_dict(). How do you think?
  3. I like the idea of processing node. In the case of multiple data processing, How about making the call_on_dict() optional for multiple dict?

Zhu

matthewcarbone commented 1 year ago

@zleung9

  1. I have a solution for this which is just not yet implemented. There should be a way to trace a child all the way back to its parents. The trouble with including previous information is that it's duplicated, and it's possible there are more than one parent in later cases.
  2. Agreed, but I think we should rethink the pipeline a bit.
  3. Agreed, but similar to point 2 we need to really think about how we want to do this.

By the way, in a little, I'm going to archive this entire repo and copy/paste operators.py into AI-multimodal/aimmdb. It would be great if you could also open a PR afterwards and add your pipeline code. It needs a bit of revision but you should own the commit history.

Thanks!

zleung9 commented 1 year ago

@x94carbone https://github.com/x94carbone Would you like to meet to discuss about this?

On Aug 16, 2022, at 8:53 AM, Matthew Carbone @.***> wrote:

@zleung9 https://github.com/zleung9 I have a solution for this which is just not yet implemented. There should be a way to trace a child all the way back to its parents. The trouble with including previous information is that it's duplicated, and it's possible there are more than one parent in later cases. Agreed, but I think we should rethink the pipeline a bit. Agreed, but similar to point 2 we need to really think about how we want to do this. By the way, in a little, I'm going to archive this entire repo and copy/paste operators.py into AI-multimodal/aimmdb. It would be great if you could also open a PR afterwards and add your pipeline code. It needs a bit of revision but you should own the commit history.

Thanks!

— Reply to this email directly, view it on GitHub https://github.com/AI-multimodal/aimm-post-processing/pull/18#issuecomment-1216596583, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD36TVY2BUQGR6LZ6TUC6B3VZOFMJANCNFSM56MHG4OA. You are receiving this because you were mentioned.

matthewcarbone commented 1 year ago

@zleung9 Sure, I'm happy to. I more or less know what needs to be done, but we'll need to consult with Joe to figure out the efficient way to do it. My overall point is that I don't think copy/pasting a bunch of metadata into derived results is the solution (but perhaps it could be in specific cases).