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.

Sign up for GitHub

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

Jump to bottom

make fragment compatible for rms #216

Merged
merged 8 commits into from
Oct 17, 2023
Merged

make fragment compatible for rms #216

merged 8 commits into from
Oct 17, 2023

Conversation

lily90502
Copy link

Make rms being able to read & draw fragments

@lily90502 lily90502 requested a review from hwpang June 8, 2023 17:32
@hwpang
Copy link
Contributor

hwpang commented Oct 4, 2023

I rebased and cleaned up unnecessary changes as @JacksonBurns and Anna need this to wrap up the fragment work.

@hwpang hwpang requested a review from JacksonBurns October 4, 2023 21:58
JacksonBurns
JacksonBurns previously approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@JacksonBurns JacksonBurns left a comment

Choose a reason for hiding this comment

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

LGTM - my only thought is that you might want to delete the commented lines, rather than carry the commented out version forward.

@hwpang
Copy link
Contributor

hwpang commented Oct 5, 2023

@JacksonBurns Thanks for catching that, I didn't notice that Lily commented those lines out. Those are changes for her personal use and should not be included in this PR. I edited that commit.

@JacksonBurns
Copy link
Contributor

@hwpang sure thing! I will re-run the tests since (from visual inspection) the errors were possibly spurious. If they fail again for the same reason, could you take a look? Tests which seem unrelated to this PR are failing.

@hwpang
Copy link
Contributor

hwpang commented Oct 5, 2023 •
edited
Loading

@JacksonBurns Thanks for rerunning the tests! The CI still failed after rerunning, due to the following error:

┌ Warning: Error requiring `EnzymeCore` from `KernelAbstractions`
│   exception =
│    LoadError: UndefVarError: EnzymeRules not defined`

I suspect that the EnzymeCore v0.1.0 and Enzyme v0.10.18 used during installation are not compatible with KernelAbstractions v0.9.8. I see that there is a Enzyme v0.11.8, so I'm attempting to relax package specification to see if that can be solved.

@hwpang hwpang requested a review from JacksonBurns October 6, 2023 16:31
@hwpang
Copy link
Contributor

hwpang commented Oct 6, 2023

I realize that the reason the CI test failed had nothing to do with EnzymeCore or Enzyme packages, but that the adjoint sensitivity results are different from the forward sensitivity results as mentioned in #235. @JacksonBurns could you re-review the PR? I think the failed CI test is not related to this PR.

JacksonBurns
JacksonBurns previously approved these changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@JacksonBurns JacksonBurns left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for figuring out the issue with the CI

mjohnson541
mjohnson541 reviewed Oct 8, 2023
View reviewed changes
Copy link
Collaborator

@mjohnson541 mjohnson541 left a comment

Choose a reason for hiding this comment

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

Thanks!

There are a few try-catches here that aren't for specific errors...that could be annoying for users...suppose someone messes up a Molecule adjlist...they won't get the actual error associated with constructing a Molecule, they will get the error associated with constructing a Fragment.

Also is it possible to add the bugfixes for #219 to this PR?

Other than that just a couple comments.

src/Simulation.jl Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@JacksonBurns
Copy link
Contributor

@hwpang can you take care of the comments from @mjohnson541's review? I'm not sure what the type cast (if it is a type cast) exactly accomplishes

@hwpang
Copy link
Contributor

hwpang commented Oct 11, 2023

@JacksonBurns Yes, I will work on this! Unfortunately I have commitments today and tomorrow, so the earliest I can work on this is Friday. In the meanwhile, you and Anna can use this branch for now for your fragment related work.

@hwpang
Copy link
Contributor

hwpang commented Oct 12, 2023 •
edited
Loading

@mjohnson541 I addressed your comments in the new push! I have an attempt to fix the issue (#219) in a previously opened PR (#230). Can you also review that PR? I think It currently failed one of the new unit tests I added. I will try getting around to fix it soon. I prefer to have separate PRs and each solves a small bug, rather than a larger PR that solves all the bugs.

mjohnson541
mjohnson541 reviewed Oct 12, 2023
View reviewed changes
Copy link
Collaborator

@mjohnson541 mjohnson541 left a comment

Choose a reason for hiding this comment

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

Looks mostly good, one last thing.

src/fluxdiagrams.jl
@@ -42,11 +42,19 @@ function drawspc(spc::Species,path::String=".")
end
end
if spc.adjlist != ""
mol = molecule.Molecule().from_adjacency_list(spc.adjlist)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do this one with Species the same way you do later?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! Done.

@hwpang hwpang force-pushed the frag_draw branch 2 times, most recently from 8e598be to b8158f6 Compare October 13, 2023 01:00
@hwpang
Copy link
Contributor

hwpang commented Oct 13, 2023 •
edited
Loading

It turns out you can't check if a Python object is of a Python class instance in Julia, which causes some part of tests to fail because isa cannot take a Python object as type. I am not sure what is the best way forward. @mjohnson541 Would love your thoughts on this.

Edit: I figured out how to call isinstance with PyCall to do the type check in the new push.

@hwpang
Copy link
Contributor

hwpang commented Oct 13, 2023 •
edited
Loading

Still trying to figure out why the changes I made for fragment would fail things for surface species...

@hwpang
Copy link
Contributor

hwpang commented Oct 14, 2023 •
edited
Loading

It turns out that the problem was with rmgmolecule. The conformer attribute was commented out in this commit, but its counterpart in the .py file was not. This causes the initialization of spc = Species() to fail with AttributeError("'molecule.species.Species' object has no attribute 'conformer'") error. I work around to not use Species in the new push, but rather to detect directly if the adjlist contains cutting label. If it doesn't, use the adjlist for Molecule, and otherwise use Fragment.

@hwpang hwpang mentioned this pull request Oct 14, 2023
Merged
@hwpang
Copy link
Contributor

hwpang commented Oct 15, 2023

The newest changes have passed the tests that we are able to pass currently. The two tests failed are related to #235. I will clean up the commits a bit.

@hwpang
Copy link
Contributor

hwpang commented Oct 15, 2023

@mjohnson541 All comments are addressed, please re-review!

@mjohnson541 mjohnson541 merged commit 49f8d1a into ReactionMechanismGenerator:main Oct 17, 2023
1 of 2 checks passed
This was referenced Nov 14, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjohnson541 mjohnson541 mjohnson541 approved these changes

@JacksonBurns JacksonBurns JacksonBurns left review comments

@hwpang hwpang Awaiting requested review from hwpang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lily90502 @hwpang @JacksonBurns @mjohnson541