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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add initial misc util functions, count, kl_div #50

Closed
wants to merge 2 commits into from

Conversation

cmvcordova
Copy link
Collaborator

No description provided.

@review-notebook-app
Copy link

Check out this pull request on聽 ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@noahweber1
Copy link
Collaborator

Hi @cmvcordova thank you for the PR.

Can you please:

  1. Link the PR to a particular ticket with explanations?
  2. How is this different then KL and other metrics introduced in this notebook? Wouldnt it be better to reconcile it there since this is the starting point? https://github.com/pinellolab/DNA-Diffusion/blob/dna-diffusion/notebooks/experiments/conditional_diffusion/easy_training_Conditional_Code_to_refactor_UNET_ANNOTATED_v4%20(2).ipynb

@cmvcordova
Copy link
Collaborator Author

cmvcordova commented Nov 18, 2022

Hey man, sure thing

  1. Create a function called KL_divergence_motifs聽#14 , Create function motif_count Input聽#13 indirectly (although as I understand it, Sameer had already implemented something similar.)
  2. These are based on the notebook indeed, however, they are being rewritten to form part of the codebase, in a comprehensive, internally consistent framework that we'll then be running experiments in.

This is the version I'll be building upon from my end

Hope this clears it up for you. In case you have more questions I'm happy to answer them.

@noahweber1
Copy link
Collaborator

It sure does and yes you did write it very thoroughly.

But then if this is suppose to be part of the codebase, why not commit it directly there (meaning not as part of the nb?)

@cmvcordova
Copy link
Collaborator Author

Thanks!

Initially, for exposure, review and making sure the functions are in agreement with what the codebase team expects. Additionally, figuring out the "hows/wheres" in the codebase, before committing a final version. Ideally, the codebase should only have working and reviewed components. Showing the code as a notebook beforehand makes this transition smoother. This is only my opinion, however and am open to further suggestions regarding the pull request pipeline.

@noahweber1
Copy link
Collaborator

noahweber1 commented Nov 18, 2022

No, it makes sense what you said.

However if we continue with this approach we run the risk that people are writing notebooks. Issuing PRs then leaving and this code never lands on the "codebase" or the main "dna-diffusion" branch.

Again this is a collective decision we make and I am fine with everything. We can gladly discuss this on saturday.

My suggestion however, since I dont see any big negatives. Is to try to marry this directly into the "codebase" branch or wait until this gets into develop.

If we go with this approach in the future, and when we have a lot of e2e running, then it will never happen that a nb implementation is good but not passing some tests.

@cmvcordova
Copy link
Collaborator Author

Great points. Let's bring them up on the Saturday meeting!

@mateibejan1
Copy link
Collaborator

Closed as solved in another PR.

@mateibejan1 mateibejan1 closed this Feb 3, 2023
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

3 participants