additions for NAR submission guidelines #54
Conversation
✅ Deploy Preview for gene-plexus ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@ChristopherMancuso You'll likely want to tweak or add things to the copy text I wrote on the home page. Also take a look at where I've added "learn more" links in the UI, and tell me if there's any other ReadTheDocs pages that could be linked in specific spots. |
|
I've been updating all the package documentation the last few days. Here is a link to the "latest" build of the doc I think this part of the documentation has the relevant info. https://pygeneplexus.readthedocs.io/en/latest/geneplexus/geneplexus.html In particular the functions listed at the top have their own specific links that might be useful to add to places |
|
Additionally a thing to add is the license files for all the data we use for this work. I have made a few license files that are all the same but different formatting. Maybe adding this information somewhere could be good too? https://github.com/krishnanlab/PyGenePlexus/blob/main/LICENSE.md |
|
It'd be helpful if you could add the licenses and license verbage that you want directly to this PR, and also note which pages/sections of the ReadTheDocs you want me to link and where in the UI. |
|
In your UI expertise, do you think any of this figure (from this manuscript) would be helpful to have anywhere on the web site? Or would just directing people to this manuscript be good enough if they want to understand more of what is going on?
|
|
I think those figures are nicely high level and could go in the UI somewhere, perhaps at the bottom of the home page or on the about page somewhere, with a link to the paper to learn more. We can also permalink to the image somehow, either in your manuscript repo or in the manuscript itself once it's published, so we don't have to duplicate the image file in this repo. |
|
Sounds good. I'll work on making a detailed list of what and where to add some of these documentation files. |
|
This link is to the journal proposal submission form, which option in cookies would we be clicking? |
|
"No cookies" (again, this is assuming we can take their word literally and they don't secretly/imprecisely also mean localStorage). Just pushed a commit to add the manuscript figure and link to the bottom of the home page, with some brief copy text. Please edit as you see fit. Note that I just link directly to the image in the manuscript. Do you foresee that manuscript ever "going down"? Do you know if Plos would ever change the url of the image? |
I like the figure down at the bottom of the home page. I don't think they would ever change the figure url so that should be a great way to do it. I was about to go through and make a list of these small changes like rewording things, where to add links to the ReadtheDocs, etc.. My plan is to do this in a google doc and share that google doc with you. Is that an OK way for me to get these changes to you? |
|
A google doc would be fine, or you can just make changes directly to the branch and I will lint/cleanup as needed. |
|
OK so since new to working on the web site code, going to do it in baby steps. I tried to update the Home page. I think I did this pretty good. The main thing is that I made some figures to be added. There is obviously some weird looking whitespace. Should I change the aspect ratio? Or is that something you can fix? Also, if you think those figures aren't good to put in there just let me know. I don't think we need to show exactly the correct output style from the results page and these "high level" figures maybe get the point across easier. |
|
I think the images and words look good. Ultimately this is your project, so it's up to you what you want to highlight. My only advice is to just treat the stuff on the home page as a very high level overview and sort of an advertisement. That said, here's some small tweaks I suggest:
A custom model trained on your genes lets you prioritize the gene list and predict genes associated with it. Fix "associated" and "genes" typo, and remove duplication of saying "list".
Compare the user trained model to known biological processes and phenotypes. At the risk of adding words just for the sake of adding words, pad this out just a little bit more so it reaches two lines like the rest of the features, and pushes the image fully to the left.
Under the hood, GenePlexus uses sophisticated methods to analyze users' input genes. Node embeddings from a six-species network are used to build features for a custom machine learning model. Learn more in the manuscript Fix "embeddings" typo. More subjectively, start with one sentence that is more "advertisement-y" and vague, setting the context for what this section on the page is about (more detailed info that you don't need to know to use the app). Then say a more specific salient point. Then defer to the manuscript for full details. Maybe remove the red "Read the manuscript" button below the figure, and instead make it an inline |
|
I'll mention some other things that I don't think that I can change
|
|
I love all your suggestions about the text changes and making the link to the manuscript inline! Are these changes you would want me to make in the code? Happy to do it, just not really great at the updating code with someone else so trying to avoid push conflicts if you are working on any of it. |
|
Just incorporated the changes and fixed those bugs you mentioned. Feel free to continue working on things, I won't be touching the code again until you change something else or point out another bug. |
|
Forgot to add the table-specific download filenames. Now I'm done. |
|
I'm pushing some commits that updated all the links and fixed some of the figures. Some more thoughts on stuff I didn't know how to change
|
We can do JPG/PNG, but I don't want to do PDF because that would require installation of a relatively heavy library. An image will be more useful anyway as it can be dragged into a word/google doc, email, whatever.
This is a new change I guess? It's not noted in the |
|
In fact, looking at that table, shouldn't Also I'm assuming that when you say something is "not supported", if either the train or result species are that, it's not supported. Please look the logic in my code for I've pushed some commits that do the following:
|
|
For the png, it doesn't seem to be saving correctly on my computer. The following is what is saved. I tried moving the network to different sizes and positions and also with auto-fit with the same results. |
|
It was a good idea to make those tables and I have added them to https://pygeneplexus.readthedocs.io/en/latest/notes/data.html. So with those added below are some thoughts on the parameter selection Some General Remarks
Specific Remarks
|
What OS and browser? If you were using Firefox, I've fixed a bug there.
Okay I see what you're saying about "combined" being just whatever of the 3 genesets are ✅s. If that's the case, I think technically fly + combined should really be a ✅ in the table. Or perhaps more appropriately, "combined" could be taken out as a row, and the note below the table could be changed to If you pass "Fly" and "Combined" to the package, will it error, or just use GO? As a user I'd hope it'd be the latter, for consistency. I think there's value in being able to have both "Combined" and "GO" for "Fly". In the UI, I may have selected "Combined", and want to cycle through other species and not have my geneset selection change. Same thing for the package: Perhaps I'm writing a script that loops through all the species, and I want to always use all available genesets available. Now I'd have to write special logic for "Fly". For this reason, I've pushed a commit to re-enable "Combined" for "Fly", and changed the secondary label to "Combo. of below". I think hopefully that makes it clear that it's a combination of whatever number of options are below it in the dropdown. It could even say "Combo. of N below" where N is dynamically the number of dropdown options minus "Combination". This seems fine enough given that it's supposed to be an advanced option, hidden by default that most people aren't meant to touch.
I don't think this matches up with the tables though? And/or is incomplete or has a typo?
If a selection has been changed automatically, we should show a notification. To keep it simple, I think everything should match exactly: the ✅/❌s in the docs tables === the enabled/disabled options === when we show a notification. I think you're saying that, in this very specific case, changing from "Combined" to "GO" is the same thing because ultimately just "GO" is used in both cases, so we don't need to show the notification. However, let's say we don't show it: the user is on "Human" + "Combined", then they switch to "Fly" which silently switches geneset to "GO", then they switch back to "Human" thinking they still have "Combined" selected but really it's "GO". All that said, I've changed the behavior of the notification. Previously, it would show when the number of filtered options was less than the total options. That is, the notification should've really been "Not all options are available for this species". Now the notification truly only shows when one of the selections changes automatically.
I've added a "Learn more" link to the notification.
This is easy to change either way, but this essentially meets the intent of something you requested in one of your UI sketches where the second species box had a "Same as input species". I thought that having the species match was probably going to be the desired setting most of the time, and that they'd usually set the input species first, knowing which genes they wanted to analyze, so they wouldn't change it to often and "overwrite" the result species (which wouldn't be a big deal anyway since there's only 7 options). |
I was using Mac OS and Safari but the Firefox fix seems like it worked for that combo as well |
|
There was a typo in the message about the errors, where I forgot to write in the word Zebrafish. The following should be correct
There is also the one if Mondo is selected and the species is change from Human to something else the error message should show up. Which you have implemented. I think you have of all this correct when the error message shows up, and also the link out to learn more is great. I think having the results species change if the training species is changed is good enough as well and let's not worry about that now. |
|
You bring up a good point about what it means to be "Combined" and how it could be good to have a Combined for every species. Right now the behavior of the package is the run will fail if Fly and Combined are input with an error message that says only GO is available for Fly. Some thoughts. The reason that there isn't Fly Combined, is that the backend data have a very specific naming structure. So if Fly just has GO, Combined would literally just be all the Fly-GO files copied and just changing GO in the filenames to Combined. I don't think having a bunch of duplicate files in the backend data that are named different would be great, right? One option is that in the package I could change the GSC to GO for the user if they input Combined-Fly and display a message/warning that says "Since GO is the only available GSC for Fly, results are generated using the Fly-GO backend files". Would something along those lines work? Then I could update the read the docs table to drop the combined row. Side note, it was a great suggestions to make this data compatibility tables. Having seen those, I will be making an explicit error message for if someone selects Mondo for a non-human species. |
|
Let me know what you want to do, but fwiw this is how I would do it: Always allow For the UI, I think I'm still going to leave the "Combined" option always there, for reasons discussed above. When it calls the backend (if you leave it the way it is now), I'll make the frontend correct it to "GO" when "Fly" is selected so it doesn't return an error. If you think this will be confusing to the user and saying "Combo. of below" in the dropdown isn't enough, I could also explicitly expand out the genesets that will be used and/or put a note explaining what "Combined" is, similar to your docs. |
Oh, I like adding the contexts used as metadata, thus Fly having Combined. I'll make the change of Fly-Combined reading in the Fly-GO files in the package, so we don't duplicate the files. I'll probably just do something like add a COMBINED_CONTEXT dictionary to the config script. This will take a few hours to code up and check. Once it is done I'll update the ML GCP function deploy scripts to use the latest commit. |
|
Yes I forgot to say, no of course I wouldn't encourage duplicating the files. I just would expect the package to handle the edge cases instead of the user having to handle them (an error only when combined + fly) |
I ended up putting this in the (?) tooltip next to the geneset context dropdown. Also added another "Learn more" link below those options linking to the readthedocs page. |
|
I got the above implemented into the main branch of PyGenePlexus in commit I'll be out of the office the rest of the week. I'll be working on stuff then still, but it will be slower. I did update the readthedocs but it is taking awhile to show up on the live site. |
|
It looks like running with Fly and Combined now works on the web server. I was playing around with it a bit more and I noticed two things
|
This is functioning as intended. There is no button to recheck because nothing has changed. This is standard UI practice. If you change the input genes, the button will reappear. And if you run a check on the exact same set of genes that you have before in the session, the result will be cached and instant.
I don't know what you mean here. I followed what you said, and that doesn't happen. If you switch to mouse, it stays as the human example genes ("BBIP1,BBS18,BBS1..."), not the example mouse genes ("Mpo,Inmt,Gnmt..."). I added some code to clear the input box when switching the train species, but only if the input box exactly matches one of the example sets. We don't want to clear it if the user has modified the example in any way. They may have pasted in their own set, and realized they selected the wrong species, and would be mad if it just cleared the the box indiscriminately. |
|
Yeah you are right, which genes are being read is working like you said it is and not how I thought it was. Also, I see now the check genes button does reappear, and that is way better than always having it there. |
There was a problem hiding this comment.
I think this is all working now and should be pretty much done. One outstanding thing is publishing the repo and then adding the to the GCP function requirement files. However, I think Arjun wants to send this and the package out for the group to review, so I will probably wait to update the requirements files until after that. So once this branch is merged we can send it out for the group to review. Does that sound good?
Closes #40 , directly addresses #40 (comment)
Closes #50