Repository navigation
Merge DataEntry definitions - #184
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
- Coverage 83.57% 83.44% -0.13%
==========================================
Files 58 58
Lines 5205 5177 -28
==========================================
- Hits 4350 4320 -30
- Misses 855 857 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| # since the test data is not mapped, but the stored data is mapped | ||
| # Revisit if DataEntry is changed to store mapped SMILES | ||
| assert row["smiles"] != entry["smiles"] | ||
| assert row["smiles"] == entry["smiles"] |
| copied_entries = [copy.deepcopy(entry) for entry in entries] | ||
|
|
||
| Returns: | ||
| The created dataset. | ||
| """ | ||
|
|
||
| for row in rows: | ||
| row["smiles_a"] = tyff.utils.molecule.map_smiles(row["smiles_a"]) | ||
| copied_entries = [entry for entry in entries] | ||
|
|
||
| if row["smiles_b"] is None: | ||
| continue | ||
| for copied_entry in copied_entries: | ||
| copied_entry["smiles"] = [tyff.utils.molecule.map_smiles(value) for value in copied_entry["smiles"]] |
There was a problem hiding this comment.
See #189 for discussion on whether or not we want this side effect
lilyminium
left a comment
lilyminium
left a comment
There was a problem hiding this comment.
Thanks Matt, LGTM from a quick skim -- just a comment on the ThermoML choices. There's also a copilot comment that seems worth resolving to me.
| if entry["tag"] == "density": | ||
| value = _predict_density(entry, observables["bulk"][keys["bulk"]]) | ||
| elif entry["type"] == "hvap": | ||
| elif entry["tag"] == "hvap": |
There was a problem hiding this comment.
I remember for the ThermoML dataset code in dimsim we had used longer type names eg enthalpy_of_vaporization. Could you please check those are consistent?
There was a problem hiding this comment.
Bit of a hornet's nest here - you remember correctly but this code may not be used in MVP at all. I'm kicking this down the road for now instead of removing or refactoring this now
Fix function signature and improve docstring format. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ge-target-classes
|
Thanks @lilyminium! |
smiles_a, etc. tosmilesandxbeing lists (maybe necessary for for Add functionality to parse osmotic coefficients #15)taginstead of shadowing built-intypeDataEntrydefinitionsDataEntryinto older code path