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

Reaction hashing not always working #4

Closed
joegilkes opened this issue Nov 4, 2023 · 2 comments
Closed

Reaction hashing not always working #4

joegilkes opened this issue Nov 4, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@joegilkes
Copy link
Collaborator

Calling KPM calculator can result in the following:

ERROR: DimensionMismatch: arrays could not be broadcast to a common size; got a dimension with lengths 9724 and 9722

where the first length is rd.nr and the second in calc.Ea. This is due to KPM identifying 2 duplicate reactions and removing them (N.B. needs an assertion in KineticaKPM for this). These reactions should have been previously identified by rhash in push!(::RxData...; unique_rxns=true) but are not, so some duplicate reactions must be generating unique hashes somehow. As this applies on fresh RxDatas reconstructed from the same reaction tree, whatever input reactions must be different enough to always randomly generate different hashes. Need to check logic for rhash construction.

@joegilkes joegilkes added the bug Something isn't working label Nov 4, 2023
@joegilkes
Copy link
Collaborator Author

The offending reactions are ["[CH2]/C=C/CCC(C)(C)C"] -> ["C=C[CH]CCC(C)(C)C"] and its reverse, which are found in the input network but not within the output KPM reactions.

It is currently unclear what KPM is considering as this reaction's 'duplicate', however this has also highlighted a problem with the KPM interface (see Kinetica-jl/KineticaKPM.jl#1)

@joegilkes
Copy link
Collaborator Author

Fixing the above issue within KineticaKPM seems to have resolved this, since there is no longer a disconnect between Kinetica inputs and KPM outputs. rhash construction is therefore fine, although this has highlighted an oversight in rhashes constructed within insert_inert!, which was fixed in ce82ea2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant