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

Added .mol functionality #290

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Added .mol functionality #290

wants to merge 5 commits into from

Conversation

nwoodweb
Copy link

@nwoodweb nwoodweb commented Jun 3, 2022

The file iodata/formats/mol.py implements V2000 *.mol compatibility, based heavily off of currently implemented *.sdf compatibility

  • I had used load_one on a phenol.mol file I had made
  • I had passed Roberto CI check

@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #290 (7f61aec) into master (73eee89) will increase coverage by 0.03%.
The diff coverage is 96.75%.

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   95.00%   95.04%   +0.03%     
==========================================
  Files          74       76       +2     
  Lines        8790     8975     +185     
  Branches     1213     1227      +14     
==========================================
+ Hits         8351     8530     +179     
- Misses        191      194       +3     
- Partials      248      251       +3     
Impacted Files Coverage Δ
iodata/formats/mol.py 91.42% <91.42%> (ø)
iodata/test/test_mol.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73eee89...7f61aec. Read the comment docs.

@FarnazH
Copy link
Member

FarnazH commented Jun 3, 2022

@nwoodweb thanks for your contributions. As shared in issue #269, having a test for every line of code is part of our contribution guidelines, so we cannot review your PR until suitable tests are added.

@nwoodweb
Copy link
Author

nwoodweb commented Jun 3, 2022

@FarnazH
Ok thankyou for clarifying.
I will add a testing script in iodata/test as well as at least 2 example files and a V2000 unit test in iodata/test/data

@nwoodweb nwoodweb closed this Jun 4, 2022
@nwoodweb nwoodweb reopened this Jun 4, 2022
add unit tests for mol implementation, roberto compliant
@nwoodweb
Copy link
Author

nwoodweb commented Jun 4, 2022

I accidentally pushed to master, which is part of the pull request, then pushed my unit tests to new-feature so I had simply merged new-feature with my forks master

I added unit test file iodata/tests/test_mol.py as well as various example mol files

@FarnazH FarnazH requested review from FarnazH and tovrstra June 7, 2022 00:10
@tovrstra
Copy link
Member

tovrstra commented Aug 3, 2022

Thanks for the contribution and my apologies for the delay. The code is almost identical to that for the SDF format. When looking for the differences, I could only find one thing related to the end marker for a single molecule:

  • For SDF this is
    M  END
    $$$$
    
  • For SDF this is
    M  END
    

Are there any other differences? If not, I'd suggest extending the SDF code to also handle the .mol case correctly. This way, we can avoid duplicated code.

@nwoodweb
Copy link
Author

nwoodweb commented Aug 4, 2022

Hi @tovrstra

.sdf and .mol are for the most part identical besides the termination string at the end. In hindsight it might be better to use branch statements to differentiate M END and M END $$$$.

However, I do remember being confused about V2000 and V3000 formatting, as well as the bond block, so I will review those before implementing your recommendation.

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