open-reaction-database / ord-data

Official data repository for the Open Reaction Database
https://open-reaction-database.org
Creative Commons Attribution Share Alike 4.0 International
219 stars 54 forks source link

Electroreductive coupling of Alkenyl and Benzyl Halides #98

Closed brilee closed 3 years ago

brilee commented 3 years ago

delano.zip

connorcoley commented 3 years ago

We should get your ord_schema PR reviewed soon since this submission uses the new macros.

brilee commented 3 years ago

re: cool to 0 with ice bath. The SI does not explicitly say "ice bath" but my understanding is that certain magic numbers mean certain cooling conditions (e.g. a -78C would indicate dry ice/acetone). 0 means ice bath to me, in a benchtop setting.

re: detailed analytical data. Would you like me to add the analytical data or skip?

brilee commented 3 years ago

delano.zip Latest supporting info for this PR

brilee commented 3 years ago

@skearnes the check_file_types test is failing but I can't figure out why

connorcoley commented 3 years ago

Thanks for adding the diastereoselective compounds!

To the description, could you also clarify that this data represents Figure 2 and Figure 3? There are also more than 24 members in this library, so the description isn't quite accurate.

re: cool to 0 with ice bath. The SI does not explicitly say "ice bath" but my understanding is that certain magic numbers mean certain cooling conditions (e.g. a -78C would indicate dry ice/acetone). 0 means ice bath to me, in a benchtop setting.

Since they don't explicitly say it though, we should probably just record that the temperature was 0 C and not make that logical connection. They could have used a dry cooling block in theory.

re: detailed analytical data. Would you like me to add the analytical data or skip?

I do think it would be nice to add an Analysis for these different types as an indication that the information might be available in the original paper. You don't have to provide the actual data or details, but noting the existence of the analyses would be worthwhile. Because the SFC was used for EE calculation for almost all of these species, copying/pasting its description into the details would be good, too.

@skearnes the check_file_types test is failing but I can't figure out why

This is likely because the comparison is made to the main branch of ord-data. This branch (#98) is now out of date, so the reaction badge svg is identified as a changed file. If you update your branch with the most recent version of ord-data/main, this should be resolved

brilee commented 3 years ago

Progress so far:

brilee commented 3 years ago

delano.zip Latest supporting info for the electrochem dataset

brilee commented 3 years ago

delano.zip with the negative float validation fix

connorcoley commented 3 years ago

Last request, I promise -- it seems like the dataset description wasn't actually updated in the dataset or notebook. Do you mind double-checking that? Then we're good to go!

brilee commented 3 years ago

My vscode has been flaky about saving ipynb files for some bizarre reason :( That edit must have gotten dropped at some point. Thanks for catching it! I believe it should be fixed now.

brilee commented 3 years ago

Any special steps required for me to merge, or should I just hit the "Merge" button?

connorcoley commented 3 years ago

Just merge!

Then, create a fresh PR to merge the branch into main. You may need to close/open that PR once or twice to get the actions to run properly (parses dataset, assigns reaction/dataset IDs, validates, counts, etc.). When all the checks pass, add me as a reviewer and I'll approve it for the final merge