AI-multimodal / aimmdb

BSD 3-Clause "New" or "Revised" License
0 stars 10 forks source link

Add multiple input operator #50

Closed CharlesC30 closed 1 year ago

CharlesC30 commented 1 year ago

First creating abstraction for postprocessing operator with arbitrary number of inputs. This will be use for an averaging operator that averages data from other entries in aimmdb.

matthewcarbone commented 1 year ago

@CharlesC30 thanks for doing this, but I really don't want to commit all this old history again. Can you try to rebase your branch onto the new master (you need to update all your local branches) and then force push your branch?

CharlesC30 commented 1 year ago

@x94carbone sorry I am not very familiar with rebasing in git. I tried just running git rebase main in my local terminal, but this had me re-merge several of the previous commits so I am guessing this is not right. Do I have to somehow rebase my fork with the original aimmdb branch?

matthewcarbone commented 1 year ago

I'd check out some docs on this. Rebasing can be a bit complicated. I'm also not entirely sure you need to but nevertheless we can't have all this duplicated history. Feel free to just make a new branch off of the most up-to-date version of the code and just re-push your changes all at once if that's easier.

One thing: you're actually merging into dev-aimm-postprocessing not main, that's probably the reason for some of your issues.

You also want to be sure you're pulling from "upstream", i.e. not your fork, you want to pull from AI-multimodal's version of the code when updating your local branches, since the original repo is almost always the source of truth.

CharlesC30 commented 1 year ago

Okay thanks. I think I understand what rebasing is supposed to do, just getting a little confused using it here. I will try something like this, but if that doesn't work then I may just make a new branch and re-push.

CharlesC30 commented 1 year ago

@x94carbone I managed to rebase with the upstream dev-aimm-postprocessing branch. There are still a couple commits left related to the tutorial notebook, since I wanted to make sure all the outputs were still cleared. But all the code looks correct so I am hoping this is okay.

matthewcarbone commented 1 year ago

@CharlesC30 this is much better, thank you!

CharlesC30 commented 1 year ago

@x94carbone one thing I wanted to ask. For testing the AverageData operator I was just averaging the pre-normalized and normalized data already in the tutorial notebook. Are there any uids we could load into the notebook that would make more sense for demonstrating the averaging/multioperators?

matthewcarbone commented 1 year ago

@CharlesC30 I'm not sure. Perhaps you could ask Eli in Slack? He might have some good ideas. At the very least we can just use "all" of the spectra originating from a single absorbing element type that you can find. After all it's just testing, so you can kinda use whatever you'd like.

CharlesC30 commented 1 year ago

@x94carbone yep, should be good to merge!