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

Allow valid permutations of molecules #1

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

sjd210
Copy link
Contributor

@sjd210 sjd210 commented Sep 9, 2024

Allows for permutations of molecules to be accepted when the allowPermutations flag is set (off by default)
e.g. CH3(CH2)8CH3 or H22C10 for C10H22 and vice-versa


NOTE: Only allows for Compound permutations of a Compound, so representing an entire Compound as a Bracket e.g. (C5H11)2 or as a Term with a different coefficient e.g. 2 C5H11 will not work.

The Compound can, however, be in any context such as within a Term and/or Statement.
e.g. CH3(CH2)8CH3 -> C8H18 + C2H4

Other changes would require a grammar rewrite, so can be handled later if wanted.


Also fixes atomCount for Expressions by keeping a separate atomTermCount and atomBracketCount to get correctly multiplied by coefficients - rather than multiplying the entire aggregate count.

This breaks many of the tests in Chemistry.test.ts, but the tests were based on the incorrect previous counting system so this is okay. I will make a card to fix these at some point later.

@sjd210 sjd210 marked this pull request as ready for review September 11, 2024 15:19
@sjd210 sjd210 changed the title Improvement/molecule permutations Allow valid permutations of molecules Sep 11, 2024
src/routes/Nuclear.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@skyepurchase skyepurchase left a comment

Choose a reason for hiding this comment

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

Looks good and nice to see a new feature being added by someone else! Not my initial thought process on how to solve the issue but I think definitely slicker.

It's a shame that atom counting was not as straight forward as I initially assumed.

I don't have time to properly test this so will leave it for proper code review (maybe someone else can learn my eccentric checker) but from the perspective of understanding how the checker works this appears sound.

src/models/Chemistry.ts Outdated Show resolved Hide resolved
src/models/Chemistry.ts Outdated Show resolved Hide resolved
@skyepurchase
Copy link
Contributor

skyepurchase commented Sep 13, 2024

The only other request I would have is to move the tagging of Elements and Compounds as bracketed from this checker to the flatten function which probably needs a rename. This takes the suboptimal AST from the parser and makes it a little more checker friendly so seems sensible to add this AST addition there.

Potentially rename it augment? Though it originally just flattened the linked-list style AST lists to javascript arrays.

@sjd210
Copy link
Contributor Author

sjd210 commented Sep 20, 2024

This set of PRs now also covers a change to recognising matching bracket types, adds/re-prioritises default checker feedback, and adds some better server logging for the sake of being able to diagnose future problems.

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