Skip to content
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

Adds a comparison table for JBrowse Jupyter features #66

Merged
merged 6 commits into from
Nov 4, 2022
Merged

Conversation

teresam856
Copy link
Contributor

  • adds a feature table to compare JBrowse Jupyter features with other tools' features

@scottcain
Copy link
Member

Adding a table like this seems like a very good idea.

@teresam856
Copy link
Contributor Author

Adding a table like this seems like a very good idea.

@scottcain Awesome. This is the last addition to the readme before I resubmit.

@garrettjstevens
Copy link
Contributor

I think the table looks good! Just a couple comments:

  • Might be good to use :heavy_check_mark: (i.e. ✔️) and :x: (i.e. ❌) instead of "yes" and "no" for easy readability. (Or unicode and if you don't want to use gitmoji).
  • It's not clear to me what "from UI" means. Do you have to use a CLI tool along with the others for those features?

@teresam856
Copy link
Contributor Author

I think the table looks good! Just a couple comments:

  • Might be good to use :heavy_check_mark: (i.e. ✔️) and :x: (i.e. ❌) instead of "yes" and "no" for easy readability. (Or unicode and if you don't want to use gitmoji).
  • It's not clear to me what "from UI" means. Do you have to use a CLI tool along with the others for those features?
    @garrettjstevens
    from ui is just more specific because say for example igv-notebook provides this https://github.com/igvteam/igv-notebook#svg-conversion. We also provide a way to get the svg of the view but you have to use the button on the component instead of the api within the jupyter notebook cell.

@cmdcolin
Copy link
Contributor

any reason to referring to "others" without specific reference?

@teresam856
Copy link
Contributor Author

any reason to referring to "others" without specific reference?

@cmdcolin No specific reason, but rather to keep it general. (We can include igv-notebook/igv js ) do we think being more specific can help?

@cmdcolin
Copy link
Contributor

yea i think it would be good to be specific :)

@ihh
Copy link
Member

ihh commented Oct 19, 2022

might be worth mentioning gos
https://github.com/gosling-lang/gos

@teresam856
Copy link
Contributor Author

might be worth mentioning gos https://github.com/gosling-lang/gos
Are we thinking of citing gos as well in the paper?

@ihh
Copy link
Member

ihh commented Oct 19, 2022

I don't think there is a paper for gos yet. If this presents an issue, maybe just leave it out for now

@ihh
Copy link
Member

ihh commented Oct 19, 2022

OK, having looked at the table:

  1. yes we ought to be specific
  2. The "others" column should be split into specific columns for IGV.js, Gos, and Mango
  3. The row "Supports other views aside from the Linear Genome View - e.g Circular Genome View" should be renamed more simply "Supports circular genome view"
  4. What does "Path support" mean exactly?

--


Supports other views aside from the Linear Genome View - e.g Circular Genome View


@teresam856
Copy link
Contributor Author

@ihh

What does "Path support" mean exactly?

So one of our limitations when it comes to local file support is that we need to pass URLs to the JBrowse embedded components rather than paths to local files just like igv-notebook handles it https://github.com/igvteam/igv-notebook#urls-and-paths . Igv-notebook's architecture allows for users to specify paths to files that exist within their mount system or file tree. On the backend they determine wether igv is running on jupyter or google colab and are able to resolve the paths. We are not able to do that with our current architecture. (correct me if I am wrong about this @garrettjstevens). The confusion was that due to an error message that was being provided by JBrowse jupyter that stated "local files were not supported" when in reality, only paths are not supported. We provide ways in which users can leverage the Jupyter server when running jupyter notebook and jupyter lab to pass the formatted urls to JBrowse. We also developed a CORS enabled and range request enabled http server to make it easier for those users who want to serve their local files but are not utilizing jupyter notebook/jupyter lab.

@ihh
Copy link
Member

ihh commented Oct 19, 2022

Ah. I thought this problem was inherent to Jupyter, and I think the text in the response to reviews currently says that too. We should correct that in the response to reviews. And as for this table, can we find a more informative descriptor than "path support"? e.g. "Paths to local files must be passed as URLs"?

@garrettjstevens
Copy link
Contributor

On the backend they determine wether igv is running on jupyter or google colab and are able to resolve the paths. We are not able to do that with our current architecture. (correct me if I am wrong about this garrett)

It looks like JupyterLab and Colab both have a way of getting the kernel to open a file (e.g. Jupyter.notebook.kernel.comm_manager.new_comm('file_request', {}) in Jupyter). Our current architecture doesn't allow opening local files with just a path, since that requires the user to go through file selection in the UI, which doesn't really fit the jbrowse-jupyter model, but it is probable we could add some sort of way to open local files. I'm not sure just yet, though, how involved that would be.

@garrettjstevens
Copy link
Contributor

I talked with @rbuels and we figured out a way we think we could open local files in Jupyter and Colab, based on the way igv-notebook does it.

The thing I mentioned above, it turns out, is actually a message channel between the client and the kernel. Jupyter's looks like Jupyter.notebook.kernel.comm_manager.new_comm, and Colab's looks like google.colab.kernel.invokeFunction. In both scenarios, you create a listener in the kernel that gets messages from the client, reads the file contents, and sends them back to the client.

The way we could connect this to JBrowse is by using internetAccounts. For example, we would create a plugin that provided a JupyterLocalFileInternetAccount. Then when the internetAccount handles the fetch, it checks to see if it's a local file, and if it is (maybe by checking to see if it uses the file:// protocol), it uses the kernel messenger instead of actually fetching the file.

I can provide more info in a pairing, etc. if needed.

@teresam856
Copy link
Contributor Author

I talked with @rbuels and we figured out a way we think we could open local files in Jupyter and Colab, based on the way igv-notebook does it.

The thing I mentioned above, it turns out, is actually a message channel between the client and the kernel. Jupyter's looks like Jupyter.notebook.kernel.comm_manager.new_comm, and Colab's looks like google.colab.kernel.invokeFunction. In both scenarios, you create a listener in the kernel that gets messages from the client, reads the file contents, and sends them back to the client.

The way we could connect this to JBrowse is by using internetAccounts. For example, we would create a plugin that provided a JupyterLocalFileInternetAccount. Then when the internetAccount handles the fetch, it checks to see if it's a local file, and if it is (maybe by checking to see if it uses the file:// protocol), it uses the kernel messenger instead of actually fetching the file.

I can provide more info in a pairing, etc. if needed.

@garrettjstevens Yeah, we can schedule something.

@ihh
I will update the table on the readme, and cite the other packages (Gos and Mango) on the manuscript. I can dedicate more time to this if we think implementing local paths would seal the deal to address the reviewer's feedback. We still have a couple weeks left before we have to resubmit.

@ihh
Copy link
Member

ihh commented Oct 21, 2022

I do think adding local file support would be good. I'm afraid I misunderstood before that this was a fundamental limitation that could not be overcome. If it can, then we sort of have to do it, I think. Thank you @garrettjstevens @teresam856 @rbuels for your flexibility looking into this. I think it is worth doing.

@teresam856 teresam856 merged commit 559f55f into main Nov 4, 2022
@cmdcolin cmdcolin deleted the comparison branch December 7, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants