-
Notifications
You must be signed in to change notification settings - Fork 13
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
Adding Levels 3 and 4 Profile Data #34
Conversation
cc @niranjchandrasekaran wooo! |
Looks great! I didn't read the code, only looked at the structure of the profiles directory. Minor edits:
I spent too much time thinking about whether we should encode the process description in the file name for
I concluded that your current approach is nice. We don't want to clutter things too much here. It is poor practice to rely on the filename (beyond a point) to encode the process. It is definitely impractical to do in some cases (e.g. variable selection). Also, I think it will be cleaner for FYI I did |
Code updated in bee9e79 - i will run the pipeline again (only the first step of counting cells, and stop it there) to recreate the cell count csv files and make sure that the updated paths work. This commit will come in at some point later today.
I agree 💯 the current approach is very readable and I am ok to keep it as is since it would take a lot of time for minimal benefit.
This is great, thank you for the note! I agree that this proposal is a more elegant solution. Let's reserve it for a future enhancement - I describe it in cytomining/pycytominer/issues/80
👍 good to know this is a specific option. In the past, I have noticed that some users of datasets embedded as git LFS files struggle to access (i think if git LFS isn't installed, pointers will be downloaded by default). We should be explicit about download instructions. I've added a note to do this in #35 |
will be added back in a future PR
I lied - will not come until next week. It is going to take some time to process the count files again with the changes specified in bee9e79 (I do agree that these changes are necessary fwiw) @shntnu - how do you feel about merging the PR with just profiles? The next steps after merging, as I see them are:
|
Sounds good to me.
👍
👍 (I did not look at the PR)
👍
👍 Could you please double check that all this, especially 1 and 4, lines up with the plan here #4 (comment)? |
Confirmed and reorganized thoughts into github project: |
if everything looks good @shntnu can you approve the PR? I will work on consensus signatures next |
Processed with pycytominer. Also adding cell counts files. Very minor modifications made to scripts updating paths according to #32 (comment)
🎉