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

use Texture for ReactionInput and Compound #698

Merged
merged 11 commits into from
Sep 20, 2023
Merged

use Texture for ReactionInput and Compound #698

merged 11 commits into from
Sep 20, 2023

Conversation

qai222
Copy link
Collaborator

@qai222 qai222 commented Aug 21, 2023

No description provided.

@qai222 qai222 requested review from skearnes and connorcoley August 21, 2023 14:02
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #698 (c03e026) into main (4961279) will increase coverage by 0.94%.
Report is 7 commits behind head on main.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
+ Coverage   69.33%   70.28%   +0.94%     
==========================================
  Files          23       23              
  Lines        2322     2423     +101     
  Branches      589      627      +38     
==========================================
+ Hits         1610     1703      +93     
- Misses        598      604       +6     
- Partials      114      116       +2     
Files Changed Coverage Δ
ord_schema/__init__.py 100.00% <ø> (ø)
ord_schema/scripts/parse_uspto.py 0.00% <ø> (ø)
ord_schema/templating.py 95.31% <100.00%> (+0.15%) ⬆️
ord_schema/validations.py 71.90% <100.00%> (+0.82%) ⬆️

... and 4 files with indirect coverage changes

@qai222
Copy link
Collaborator Author

qai222 commented Aug 21, 2023

addressing #696

Copy link
Collaborator

@connorcoley connorcoley left a comment

Choose a reason for hiding this comment

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

Steve and I were talking about this and had some thoughts:

  • The MixtureDescription field seems a bit unnecessary. I get wanting to understand the "texture/phase" (for lack of a better phrase) from the perspective of processability, so we should include it somehow
  • Perhaps StateOfMatter could be generalized a bit more to includeSLURRY and other ways that you would describe a potential input. This would be the singular enum we would use to record this information
  • Probably don't need plasma, lol
  • Having this kind of texture/state/phase defined at the Compound, CrudeComponent, and ReactionInput level seems okay. We would need to expand the validations for this to (1) ensure details are given if type is CUSTOM, and (2) make sure that single-compound inputs have the same texture/state/phase as the compound when both are defined
  • CUSTOM should always be the second enum option after UNSPECIFIED, and all enums should have a CUSTOM option (probably including this texture/state/phase
  • Is there any world in which we simply expand the product texture field and use it for the ProductCompound, Compound, CrudeComponent, and ReactionInput fields?

@qai222
Copy link
Collaborator Author

qai222 commented Aug 28, 2023

Thanks for the comments.

  • The MixtureDescription field seems a bit unnecessary. I get wanting to understand the "texture/phase" (for lack of a better phrase) from the perspective of processability, so we should include it somehow
  • Perhaps StateOfMatter could be generalized a bit more to includeSLURRY and other ways that you would describe a potential input. This would be the singular enum we would use to record this information
  • Is there any world in which we simply expand the product texture field and use it for the ProductCompound, Compound, CrudeComponent, and ReactionInput fields?

My idea was just to expand texture but it feels really weird to put "dispersion" or "gas" there. A more suitable term may be morphology or simply form. But I'm not sure if changing message names would break compatibility.

Another thing is the current options of texture are not disjoint (ex. a powder, as a solid, can also be a crystal). I feel two enum each with three disjoint options and clear physical meaning are often better than one enum of >9 options.

  • CUSTOM should always be the second enum option after UNSPECIFIED, and all enums should have a CUSTOM option (probably including this texture/state/phase
  • Having this kind of texture/state/phase defined at the Compound, CrudeComponent, and ReactionInput level seems okay. We would need to expand the validations for this to (1) ensure details are given if type is CUSTOM, and (2) make sure that single-compound inputs have the same texture/state/phase as the compound when both are defined

Got it, will do this after we settle the design.

  • Probably don't need plasma, lol

Only include for completeness... although this is in a recent JACS Plasma Electrochemistry for Carbon–Carbon Bond Formation via Pinacol Coupling
image

(hope we don't need to describe excited states in the future)

@qai222
Copy link
Collaborator Author

qai222 commented Sep 6, 2023

todo: throw a warning for a ReactionInput when

  1. there is only one Compound, and
  2. ReactionInput.texture is different from Compound.texture

@qai222 qai222 requested a review from connorcoley September 8, 2023 19:48
ord_schema/validations.py Outdated Show resolved Hide resolved
proto/reaction.proto Outdated Show resolved Hide resolved
ord_schema/validations.py Show resolved Hide resolved
- fix bug in texture type validation
- add validation test for texture type consistence in `ReactionInput`
@qai222 qai222 requested a review from connorcoley September 8, 2023 21:29
@qai222
Copy link
Collaborator Author

qai222 commented Sep 14, 2023

Moving Texture out from ProductCompound seems backward compatible.

$ python validate_dataset.py --input=ord_dataset-e984b2d4813f44d59867e1771acc9b66.pb.gz
INFO 2023-09-14 07:19:11,320 validate_dataset.py:49: Found 1 datasets
INFO 2023-09-14 07:19:11,320 validate_dataset.py:54: Validating ord_dataset-e984b2d4813f44d59867e1771acc9b66.pb.gz
INFO 2023-09-14 07:19:14,273 validations.py:122: Validation summary for ord_dataset-e984b2d4813f44d59867e1771acc9b66.pb.gz: (720,)/720 successful (0 failures)

@qai222
Copy link
Collaborator Author

qai222 commented Sep 14, 2023

4 tests failed, all seem unrelated to ord_schema but come from npm install jest@^29.3.1 which requires a non-existing version of babel/types@^7.22.18 (latest 7.22.17).

@qai222
Copy link
Collaborator Author

qai222 commented Sep 15, 2023

4 tests failed, all seem unrelated to ord_schema but come from npm install jest@^29.3.1 which requires a non-existing version of babel/types@^7.22.18 (latest 7.22.17).

This issue has been resolved in jest@29.7.0,all checks passed.

proto/reaction.proto Outdated Show resolved Hide resolved
proto/reaction.proto Outdated Show resolved Hide resolved
@skearnes
Copy link
Collaborator

please update the PR description; only deals with Texture now

ord_schema/__init__.py Outdated Show resolved Hide resolved
- additional comment for `Texture`
- matter state enum for validation
@qai222 qai222 changed the title add mixture description and state of matter use Texture for ReactionInput and Compound Sep 20, 2023
@qai222 qai222 merged commit 07f1fb4 into main Sep 20, 2023
18 checks passed
@qai222 qai222 deleted the input_phase branch September 20, 2023 21:17
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.

3 participants