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

feedback changes for #260 #5

Merged
merged 2 commits into from
Sep 18, 2024
Merged

Conversation

manulera
Copy link

Hi @JeffXiePL,

Here are some changes that I have made. This was really helpful. It has been great to have someone with fresh eyes to look at the code for the first time and put together a getting started set of notebooks.

I have streamlined and re-organised some of them. From the diffs, I am not sure it will be easy to see where things have gone from where you put them, but if you look ath them side by side, you can see that most of what you wrote is still there, but in a slightly different form.

Some of the things I have done:

  • Rename some of the files (e.g. sequence.gb to U49845.gb and same for the FASTA, to have more meaningful name)
  • In some places, when you were using the list output of parse and consistently used [0] for the first element, I changed it to use the element directly. In general, if a list only has one element, it's better to store that element in a variable instead.
  • I have removed references to files being present in the path. This is the assumed default, so there is no need to specify it.
  • I have tried to use more presice language sometimes: "Show your GenBank file in pyton" vs. "Convert the Dseqrecord object into a formatted string in GenBank format"

If you have some time to keep working on this, I have left a couple of todo-manu comments in the Example_Restriction.ipynb notebook. You can have a go at those if you have the time. Otherwise, let me know and I will finish that.

Two important misconceptions that I noticed:

  • In Dseq_Features.ipynb, you mention that in the Location objects, the start and end are 1-based and that intervals are closed. Instead, Location objects are like python ranges. See the updated notebook.
  • The values in the qualifiers dictionary of a feature should be lists, not strings. For instance, see the section "Best practices for qualifiers" in the updated Dseq_Features.ipynb notebook.

Thanks again, and let me know if you will work on the TODOs in the Example_Restriction.ipynb notebook.

@JeffXiePL
Copy link
Owner

Hello Manu:

Thanks for taking a look at this! I'll works on to-dos on the Example_Restrictions.

Peilun

@JeffXiePL JeffXiePL merged commit d6c360f into JeffXiePL:docs_peilun Sep 18, 2024
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 -> 15

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.

2 participants