feat: Incorporate "Gas Scheme" feature in reaction variations table #2250

JanCBrammer · 2024-12-03T11:41:19Z

This PR addresses #2213.
See also https://docs.google.com/document/d/1Jd6V5zqRv0cfw_xAJE-oHDYzSuBMQyBBz3wQQBxRGJo/edit and https://www.chemotion.net/docs/eln/ui/elements/reactions?_highlight=gas#gas-phase-reaction-scheme.

The review is partly conducted here: https://docs.google.com/document/d/1b9zMBb4yf_we1B7tal7MAS4FlZtWq_urM-8tuvUA-8Q/edit?tab=t.0.

app/api/entities/reaction_variation_entity.rb

github-actions · 2024-12-03T11:42:48Z

lib/reporter/docx/detail_reaction.rb

@@ -60,23 +60,25 @@ def variations
            'startingMaterials' => variation_materials(var, :startingMaterials),
            'reactants' => variation_materials(var, :reactants),
            'solvents' => variation_materials(var, :solvents),
-            'products' => variation_products(var),
+            'products' => variation_materials(var, :products),


Metrics/ClassLength: Class has too many lines. [626/200]

github-actions · 2024-12-03T11:42:48Z

lib/reporter/docx/detail_reaction.rb

-            (v[:aux][:isReference] ? '; Ref' : '')
-        end
-      end
+        variation[type].map do |_, vi|


Metrics/CyclomaticComplexity: Cyclomatic complexity for variation_materials is too high. [8/7]

github-actions · 2024-12-03T12:03:01Z

LCOV of commit `807bd3d` during Continuous Integration #4189

Summary coverage rate:
  lines......: 65.5% (14049 of 21453 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

github-actions · 2024-12-18T10:54:05Z

LCOV of commit `2efc953` during Continuous Integration #4258

Summary coverage rate:
  lines......: 66.7% (14339 of 21495 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

adambasha0

Hi Jan, Thanks for the feature. I posted some suggestions and potential bug that you can check later.

adambasha0 · 2024-12-18T15:02:06Z

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.

-ActiveRecord::Schema.define(version: 2024_07_26_064022) do
+ActiveRecord::Schema.define(version: 2024_11_29_093956) do


is the timestamp change needed in this PR? - updating the timestamp might seem unnecessary

Changing the timestamp is meant to ensure that we run the migration that's necessary for this PR to work.
See db/migrate/20241129093956_add_gas_materials_to_reaction_variations.rb.

adambasha0 · 2024-12-20T10:32:34Z

db/migrate/20241129093956_add_gas_materials_to_reaction_variations.rb

+      variations.each do |variation|
+        SAMPLES_TYPES.each do |key|
+          variation[key].each_value do |material|
+            material['aux'].delete('materialType')


many repeated code in this block, especially: material.delete
suggestion: combine all keys to be deleted in one array and loop over the array to delete elements in a separate method
keys_to_delete = %w[duration temperature concentration turnoverNumber turnoverFrequency]

I feel like a one-off migration is not the place to save LOC.
In this case I believe it doesn't hurt to keep the code explicit.

app/api/entities/reaction_variation_entity.rb

adambasha0 · 2024-12-20T12:02:48Z

db/migrate/20241129093956_add_gas_materials_to_reaction_variations.rb

+end
+
+class AddGasMaterialsToReactionVariations < ActiveRecord::Migration[6.1]
+  def up


I am unsure what these migrations are used for, especially the down migration. can you explain the design concept here?

The migration is modifying the variations column of the reaction table.
Note that the migration does not add or remove columns in the table.
Instead it updates the existing data in the variations column to accommodate the gas feature.
Accommodating the gas feature consists of adding new entries to materials (such as aux.gasType).
It also means some restructuring of the materials' data to make the overall data structure more uniform.
The down migration undoes all changes related to the gas feature.

how did it exist at the first place? - the gas phase feature was not there before

I think I don't understand the question. What's "it" referring to?

how come these attributes (such as: turnoverFrequency, turnoverNumber, concentration, temperature, duration, and gasType) are deleted from the reactions variations, if there are not supposed to be there at the first place?

The attributes related to the gas feature are deleted in the down migration because the down migration is reverting the up migration where they're added.

adambasha0 · 2024-12-20T12:07:35Z

db/migrate/20241129093956_add_gas_materials_to_reaction_variations.rb

+            material.delete('duration')
+            material.delete('temperature')
+            material.delete('concentration')
+            material.delete('turnoverNumber')


isn't it better to adhere to ruby's standard naming convention about using snake case and also to match the attribute names in the the gas_phase_data column in reactions_samples table?

The reaction-variations data-structure is exclusively handled on the client (JS) side, that's why I stuck with camelCase.

adambasha0 · 2024-12-20T12:20:14Z

spec/api/entities/reaction_entity_spec.rb

@@ -27,158 +27,182 @@
        variations: [{ id: '1',
                       analyses: [1, 2],
                       notes: '',
-                       products: { '47': { aux: { yield: 0,


maybe it would be nice to have factory reaction_with_variations defined in factories/reactions.rb. This would make it easier to create reactions with variations consistently

Good point, I avoided doing this so far tbh. See bf86a76.

Ok, good. Did I miss the utilization of reaction_with_variations to test the functionality of updating a value or unit (e.g., in the gas phase) and verifying the recalculation of affected material values or units? Additionally, do we have a validation mechanism in the backend to ensure the accuracy of values submitted from the client to the server before they are saved in the database?"

Did I miss the utilization of reaction_with_variations to test the functionality of updating a value or unit (e.g., in the gas phase) and verifying the recalculation of affected material values or units?

Do the tests under spec/javascripts/packs/src/apps/mydb/elements/details/reactions/variationsTab/ cover what you have in mind?

ensure the accuracy of values submitted from the client to the server before they are saved in the database?

What do you mean with "accuracy"?

adambasha0 · 2024-12-20T14:35:19Z

...cript/src/apps/mydb/elements/details/reactions/variationsTab/ReactionVariationsComponents.js

+}
+
+function MaterialFormatter({ value: cellData, colDef }) {
+  const { currentEntry, displayUnit } = colDef.entryDefs;


can you check this error? , it happens when I click on duration button: cellData undefined

Do you mean the duration entry of a gaseous product?

What data resulted in the error? For existing data, you might need to run the migration (the one that's part of this PR).

github-actions · 2025-01-09T11:42:52Z

lib/reporter/docx/detail_reaction.rb

@@ -60,23 +60,25 @@ def variations
            'startingMaterials' => variation_materials(var, :startingMaterials),
            'reactants' => variation_materials(var, :reactants),
            'solvents' => variation_materials(var, :solvents),
-            'products' => variation_products(var),
+            'products' => variation_materials(var, :products),


Metrics/ClassLength: Class has too many lines. [627/200]

github-actions · 2025-01-09T12:03:00Z

LCOV of commit `bf86a76` during Continuous Integration #4290

Summary coverage rate:
  lines......: 66.9% (14431 of 21587 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

github-actions · 2025-01-24T07:10:56Z

lib/reporter/docx/detail_reaction.rb

-            (v[:aux][:isReference] ? '; Ref' : '')
-        end
-      end
+        variation[type].map do |_, vi|


Metrics/CyclomaticComplexity: Cyclomatic complexity for variation_materials is too high. [8/7]

github-actions · 2025-01-24T07:30:13Z

LCOV of commit `90944aa` during Continuous Integration #4353

Summary coverage rate:
  lines......: 66.9% (14443 of 21581 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

github-actions · 2025-01-24T12:30:18Z

LCOV of commit `63c42a7` during Continuous Integration #4360

Summary coverage rate:
  lines......: 66.8% (14426 of 21582 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

adambasha0 · 2025-01-30T10:39:46Z

Chemotion.dev.13.webm

adambasha0 · 2025-01-30T11:21:04Z

Chemotion.dev.14.webm

adambasha0 · 2025-01-30T11:27:20Z

Chemotion.dev.15.webm

adambasha0 · 2025-01-30T13:41:31Z

sometimes there is miscalculations:
Chemotion dev (16).webm

JanCBrammer

The review comment pertaining to Chemotion.dev.13.webm and Chemotion.dev.14.webm is:
"when I tried to change the value mmol of feedstock, it did not get changed (sometimes change happens, sometimes not)"

The sorting is enabled for the column that's being changed.
This is why the edited row shifts as soon as the edit is concluded.
Observe how the row IDs shift as soon as the cell edit is concluded.

Unrelated to (but inspired by) the review comment, I observed that the column sorting is in lexicographic order (1, 10, 2), not, as expected in numeric order (1, 2, 10).
I changed the sorting from lexicographic to numeric in 126d6b5.

adambasha0 · 2025-01-31T09:04:31Z

The review comment pertaining to Chemotion.dev.13.webm and Chemotion.dev.14.webm is: "when I tried to change the value mmol of feedstock, it did not get changed (sometimes change happens, sometimes not)"

The sorting is enabled for the column that's being changed. This is why the edited row shifts as soon as the edit is concluded. Observe how the row IDs shift as soon as the cell edit is concluded.

Unrelated to (but inspired by) the review comment, I observed that the column sorting is in lexicographic order (1, 10, 2), not, as expected in numeric order (1, 2, 10). I changed the sorting from lexicographic to numeric in 126d6b5.

Hi Jan, thanks for the feedback. I did not notice this. could you provide some info about the arrows and x buttons made for the order functionality on hover over? .. it is not clear to the user what are these buttons expected to d

JanCBrammer

The review comment pertaining to Chemotion.dev.15.webm is:
"When I select one field in a column, and then click on a column header to switch the header >> this leads to false behavior where the initial field value is intact and shown for the wrong column header. Also clicking somewhere else in the table leads to an error"

I fixed the problem in d97c2c6 according to https://www.ag-grid.com/react-data-grid/cell-editing-start-stop/#stop-editing-when-grid-loses-focus.

adambasha0 · 2025-01-31T09:05:00Z

styling issues: happens on reload of the page
Chemotion dev (17).webm

JanCBrammer · 2025-01-31T16:02:34Z

styling issues: happens on reload of the page

Fixed with 52ad4e1.

JanCBrammer · 2025-01-31T16:08:08Z

sometimes there is miscalculations

In conversation with @adambasha0, we identified the missing calculation:
A gas material's TON and TOF need to be updated when a catalyst material's amount (mol) changes. The reactive update of the gas materials' TON and TOF are implemented in 6353375.

JanCBrammer

@adambasha0, I've fixed all functional problems you've pointed out, thanks for the thorough review! I'll defer all request regarding styling to the next round of "cosmetic" updates to not bloat this PR any further.

The example of the reaction variations data structure is unnecessarily verbose and prone to be outdated.

Prior to this commit the formatters returned strings, which caused sorting to be in lexicographic, not, as expected, in numeric order.

github-actions bot reviewed Dec 3, 2024

View reviewed changes

adambasha0 self-requested a review December 13, 2024 10:41

adambasha0 assigned JanCBrammer Dec 13, 2024

adambasha0 added feature enhancement labels Dec 13, 2024

JanCBrammer linked an issue Dec 17, 2024 that may be closed by this pull request

Reaction Variations: Incorporate "Gas Scheme" feature #2213

Open

JanCBrammer force-pushed the reaction-variations-gas-phase branch from 807bd3d to 2efc953 Compare December 18, 2024 10:31

JanCBrammer force-pushed the reaction-variations-gas-phase branch from 2efc953 to c02afd0 Compare December 19, 2024 06:27

adambasha0 requested changes Dec 20, 2024

View reviewed changes

JanCBrammer force-pushed the reaction-variations-gas-phase branch from c02afd0 to bf86a76 Compare January 9, 2025 11:41

github-actions bot reviewed Jan 9, 2025

View reviewed changes

PiTrem added this to the v2.0.0 milestone Jan 21, 2025

JanCBrammer force-pushed the reaction-variations-gas-phase branch from bf86a76 to 90944aa Compare January 24, 2025 07:09

github-actions bot reviewed Jan 24, 2025

View reviewed changes

JanCBrammer commented Jan 31, 2025

View reviewed changes

JanCBrammer added 28 commits February 4, 2025 08:37

Move components to dedicated file

04f4211

Rename file

e635b33

Set columnDefinitions with useReducer hook

e487aaa

Remove local state reactionVariations

a5f6f80

Fix formatting after rebase

80fed62

Change editability of duration according to gas mode

c276a4a

Enable updates of column group properties

4234944

Update columnDefinitions on gasMode change

5be39a9

Update materials' gas type in columnDefinitions

edd47d9

Enable representation of gaseous materials

a14a1e0

Add gas parser

560fe3a

Inline function

515d5e9

Move functions

713c8e1

Add feedstock parser

40e5e3f

Test and refactor gas parser

fc21a29

Test helper functions

23db085

Adapt entities to gas types

de261b3

Adapt report generation to gas types

5930ae4

Shorten comment

0a0b51b

The example of the reaction variations data structure is unnecessarily verbose and prone to be outdated.

Migrate to material-specific entries

4e75dfe

Update changelog

7627af8

Extract predicates

feac292

Add factory for reaction with variations

f4eefbf

Resolve merge conflicts

847c040

Show all material entries in overlay

832771c

Return numbers from formatters

b88f4ae

Prior to this commit the formatters returned strings, which caused sorting to be in lexicographic, not, as expected, in numeric order.

Stop editing when cells loose focus

d97c2c6

Auto-size columns when tab is (re-)entered

52ad4e1

JanCBrammer force-pushed the reaction-variations-gas-phase branch from f72fa0a to 52ad4e1 Compare February 4, 2025 08:43

Update gas material on catalyst material change

6353375

feat: Incorporate "Gas Scheme" feature in reaction variations table #2250

Are you sure you want to change the base?

feat: Incorporate "Gas Scheme" feature in reaction variations table #2250

Conversation

JanCBrammer commented Dec 3, 2024 • edited Loading

github-actions bot Dec 3, 2024

Choose a reason for hiding this comment

github-actions bot Dec 3, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2024

LCOV of commit 807bd3d during Continuous Integration #4189

github-actions bot commented Dec 18, 2024

LCOV of commit 2efc953 during Continuous Integration #4258

adambasha0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambasha0 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambasha0 Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanCBrammer Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Jan 9, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2025

LCOV of commit bf86a76 during Continuous Integration #4290

github-actions bot Jan 24, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2025

LCOV of commit 90944aa during Continuous Integration #4353

github-actions bot commented Jan 24, 2025

LCOV of commit 63c42a7 during Continuous Integration #4360

adambasha0 commented Jan 30, 2025 • edited Loading

adambasha0 commented Jan 30, 2025

adambasha0 commented Jan 30, 2025

adambasha0 commented Jan 30, 2025

JanCBrammer left a comment • edited Loading

Choose a reason for hiding this comment

adambasha0 commented Jan 31, 2025

JanCBrammer left a comment • edited Loading

Choose a reason for hiding this comment

adambasha0 commented Jan 31, 2025

JanCBrammer commented Jan 31, 2025 • edited Loading

JanCBrammer commented Jan 31, 2025 • edited Loading

JanCBrammer left a comment

Choose a reason for hiding this comment

JanCBrammer commented Dec 3, 2024 •

edited

Loading

LCOV of commit `807bd3d` during Continuous Integration #4189

LCOV of commit `2efc953` during Continuous Integration #4258

adambasha0 Dec 20, 2024 •

edited

Loading

adambasha0 Jan 10, 2025 •

edited

Loading

JanCBrammer Jan 10, 2025 •

edited

Loading

LCOV of commit `bf86a76` during Continuous Integration #4290

LCOV of commit `90944aa` during Continuous Integration #4353

LCOV of commit `63c42a7` during Continuous Integration #4360

adambasha0 commented Jan 30, 2025 •

edited

Loading

JanCBrammer left a comment •

edited

Loading

JanCBrammer left a comment •

edited

Loading

JanCBrammer commented Jan 31, 2025 •

edited

Loading

JanCBrammer commented Jan 31, 2025 •

edited

Loading