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

Modify package structure #6

Merged
merged 24 commits into from
Nov 10, 2020
Merged

Modify package structure #6

merged 24 commits into from
Nov 10, 2020

Conversation

Midnighter
Copy link
Collaborator

This introduces the new package structure and configuration as we have discussed @vishnumahamkali

I've tried to fix the example but the tmodel constructor changed quite a bit and I'm not completely sure how to adapt it.

@Midnighter Midnighter self-assigned this Nov 5, 2020
@@ -293,10 +290,10 @@ def core_stoichiometry(self):
return (rxn_var_name, stoichiometry_core)

def update_thermo_variables(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a better way to check if the new variables are added or not ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this pull request is only meant to upgrade the package structure, I'd like to tackle this question in a different issue/pull request, if that's okay?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, make sense. thanks

for metabolite in self.metabolites:
if metabolite.Kegg_id in metabolite_accessions:
accessions[metabolite.id] = metabolite_accessions[
accessions = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this function broke when the cache file was removed. As a temp fix I made this change. Could you check the function to properly do the cache file location

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similarly to the previous comment I'd like to tackle this separately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, At the moment it can't create a cache file, gets from equilibrator-api every time. I will comment out the cache file writing line for now to avoid error

Copy link
Collaborator

@vishnumahamkali vishnumahamkali left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thank you

@Midnighter
Copy link
Collaborator Author

Awesome 🙂

@Midnighter Midnighter merged commit a4fd3e6 into master Nov 10, 2020
@Midnighter Midnighter deleted the chore-structure branch November 10, 2020 22:55
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