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

unable to create 4-carbon chain #148

Closed
arouinfar opened this issue Jan 24, 2020 · 22 comments
Closed

unable to create 4-carbon chain #148

arouinfar opened this issue Jan 24, 2020 · 22 comments
Assignees

Comments

@arouinfar
Copy link
Contributor

Noticed during #131

It should be possible to connect four carbons in a straight chain, but that's not currently possible.
4-c

However, it is possible to connect them in this arrangement
image

@Denz1994
Copy link
Contributor

This issue is due to structuresData.js not containing the valid structure for C4 as-built above.

More details regarding implementation:

The allowed structures are validated every time and atom is attempted to bond (see Kit.attemptToBondMolecule). From this, the attempted bond is validated against the master list structuresData.js of allowed structures via a hash string. If the structure that is built from the molecule bonds (StrippedMolecule) doesn't exist in that master list, then it is rejected.

The workaround is to amend the structuresData.js with the entry for C4 as it is built and bonded above. @jonathanolson didn't see why this structure shouldn't exist and perhaps this is related to the need to regenerate new data for the structures (similar to #158). There is a sanity check to prevent looping but that didn't seem promising when removing the check (see MoleculeStructure.hasLoopsOrIsDisconnected)

@arouinfar Can you confirm that C4 can be created in master?

@Denz1994
Copy link
Contributor

Note: The above measures to prevent looping were implemented to prevent structures like this.

image

@arouinfar
Copy link
Contributor Author

@Denz1994 something's still not quite right. I can connect 4 carbons in a straight line, but I'm then unable to attach any hydrogens to the carbon chain UNLESS I first attach something else like an N or O.

C4

I'm also finding cases like this that I can make in Java, but not in HTML5:

However, the data for this molecule can be found in OtherMoleculesData.js
acetic acid ethyl ester|C4H8O2|8857

@Denz1994
Copy link
Contributor

The structure data has been inputted for the above-mentioned molecules. Can you confirm this is the case on Master @arouinfar?

@arouinfar
Copy link
Contributor Author

Things are generally working now @Denz1994. I'm seeing some buggy behavior that I'm not 100% sure how to consistently reproduce. Occasionally, I am unable to create large molecules, and it seems like it happens if I've used the button to cut all bonds

image

Can you play around with it a bit and see if you can figure out what's going on? I also recommend having QA bang away on this issue during the next dev test.

@arouinfar arouinfar assigned Denz1994 and unassigned arouinfar Mar 30, 2020
@arouinfar
Copy link
Contributor Author

arouinfar commented Mar 30, 2020

@Denz1994 something strange is going on. I'm currently trying to verify #169, and was trying to build acetic ethyl acid ester from #148 (comment). I was able to build it earlier today on master (see the screenshot in #169), but now I don't seem to be able to.

My current procedure is:

  1. Open Playground and scroll to 2nd page of carousel.
  2. Make C-C-O-C by dragging atoms out of the bucket and immediately connecting them to the chain.
  3. Try to connect a 4th carbon to the end of the chain to make C-C-O-C-C but it bumps away like it's an illegal structure.

EDIT: I was able to get around this by adding the 2nd oxygen before trying to connect the 4th carbon. The order in which atoms are connected should not matter.

Alternative step 3:

Adding 4th carbon is now possible:

@Denz1994
Copy link
Contributor

Denz1994 commented Apr 8, 2020

Regarding #148 (comment):

A testing cycle may look something like this:

  1. Build a specific molecule from a set of large molecules, while tracking the order by which the molecule was built.
  2. Break the molecule using the blue arrowed button
  3. Reconstruct the molecule from step 1, using a different build order.
  4. Repeat 1-3 for the remaining set of large molecules.

*The point is the order of the build path shouldn't matter to construct the molecule

I've tried these steps with the below molecules and can't replicate the concerns brought up #148 (comment). @arouinfar Should we add more molecules to test? Also, what do you think about the above-proposed testing cycle?

Here are the molecules I tested with:
image

image

image

image

@Denz1994
Copy link
Contributor

Denz1994 commented Apr 8, 2020

@arouinfar Could you try this dev version for reviewing #148 (comment). The build order for acetic ethyl acid ester shouldn't matter in this build.

@Denz1994
Copy link
Contributor

I misinterpreted the check that prevents #148 (comment). This was allowed in the legacy version as well.

The concern was regarding a user no knowing what atoms are bonded in the example in #148 (comment). However, with the addition of the cut targets (yellow circles), it clearly shows what atoms are bonded. I don't think this is an issue. Tagging @arouinfar to confirm.

@Denz1994
Copy link
Contributor

Denz1994 commented May 21, 2020

Also, the above commits should have fixed the order dependent bonding problem that was mentioned by @arouinfar above. Can you test in this dev version @arouinfar?

@jonathanolson and I did some intense deep-diving into the visitor arrays in checkEquivalency(). It looks like I was removing the incorrect items from the visited atoms causing these false equivalencies.

Dev version: https://phet-dev.colorado.edu/html/build-a-molecule/1.0.0-dev.74/phet/build-a-molecule_en_phet.html

@Denz1994 Denz1994 assigned arouinfar and unassigned KatieWoe and Denz1994 May 21, 2020
Denz1994 added a commit that referenced this issue May 21, 2020
@arouinfar
Copy link
Contributor Author

This appears to be fixed in dev.74 @Denz1994.

If the sim is in a good place, it would be nice to publish a new prototype to the website.

@Denz1994
Copy link
Contributor

Denz1994 commented Jun 1, 2020

This issue has been listed for QA to test with specific instructions in #148 (comment).

@KatieWoe
Copy link
Contributor

KatieWoe commented Jun 9, 2020

I noticed (using phetsims/qa#506) that, if the molecule is built in a certain way, there may not be room to add some of the atoms needed. Is this acceptable?
Trying to build this in a slightly different configuration:
cantfinishthis
No room to add last atoms:
notfinished

@Denz1994
Copy link
Contributor

Correct me if I'm wrong @arouinfar, but I think the second image in #148 (comment) is technically a different molecule. For instance, the center carbon doesn't have two fluorines bonded to it.

If that is the case then this is acceptable.

@KatieWoe
Copy link
Contributor

It is a different molecule, but I was trying to build the top one. That second Fluorine can't be added because there is no room for it, it just bounces off. That is what I was trying to report.

@Denz1994
Copy link
Contributor

Ah, I see now. I think this is more in line with the atom layering issue. I don't think this is necessarily buggy behavior anymore because the user picked a path to build the molecule in a way that makes them run out of space. This is different than having the space to build and the correct atoms and it failing.

I'll leave to @arouinfar for input.

@Denz1994 Denz1994 assigned arouinfar and unassigned arouinfar and Denz1994 Jun 10, 2020
@arouinfar
Copy link
Contributor Author

I don't think this is necessarily buggy behavior anymore because the user picked a path to build the molecule in a way that makes them run out of space. This is different than having the space to build and the correct atoms and it failing.

Agreed @Denz1994. I think it's important for students to thoughtfully place their atoms when building large molecules. I don't see the current behavior as a bug, but rather a productive constraint.

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

No branches or pull requests

3 participants