added test case for initial cycles oom

[uli-f]

test case for #976

[johnmay]

Ah it's another bug when trying to actually generate them, I'll try and find time to take a look. Thanks.

[johnmay]

Hi @uli-f,

Just letting you know this, and the RDF, and the SMIRKS bits are on my radar. Doing a 2.9 release but I will try and get round to getting these bits fixed/merged in soon. Sorry it's taken so long, just too much to do :-)

[uli-f]

Hi @uli-f,

Just letting you know this, and the RDF, and the SMIRKS bits are on my radar. Doing a 2.9 release but I will try and get round to getting these bits fixed/merged in soon. Sorry it's taken so long, just too much to do :-)

Thanks @johnmay, that is good news! I very much appreciate all the work you put into CDK.

With the RDF bits I assume you are referring to #942 ? If so please let me know if there is anything I can do on my side to get this merged. It is a bit half-baked because I wanted to get some feedback before continuing...

[johnmay]

With the RDF bits I assume you are referring to #942 ? If so please let me know if there is anything I can do on my side to get this merged. It is a bit half-baked because I wanted to get some feedback before continuing...

Yep, that's fine. I actually need this for a project at NextMove now so that should move forward. At ACS San Francisco next week so busy for next few weeks but after that should move forward relatively quickly.

[johnmay]

Need to modify the test to check for the exception now.

[egonw]

Need to modify the test to check for the exception now.

yes, I just saw that:

[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   AromaticityTest.outOfMemoryExceptionInitialCycles:178 » Intractable Too many relevant cycles cycles! max=512000 was=16777240. Increase this limit with System property -Dcdk.maxRelevantCycles=<num>
[INFO] 
[ERROR] Tests run: 1005, Failures: 0, Errors: 1, Skipped: 0

[egonw]

@johnmay, are you fixing this? or shall I?

[johnmay]

Well on this branch needs to be @uli-f - but we can send a PR to the PR if that makes sense.

Just needs an Assertions.assertThrows()

[egonw]

but we can send a PR to the PR if that makes sense.

Ah, we don't have that option enabled? Let me check ...

[egonw]

Ah, we don't have that option enabled? Let me check ...

@johnmay, yes, it was enabled. I have just made the extra patch.

[johnmay]

Missing an import I think, how did you do the PR and have it appear automatically?

I needed to do that the Ertl Functional Group stuff

[johnmay]

Oh you just push to their branch, odd.

[johnmay]

Nope I don't understand,

 ! [remote rejected]       uli-f-oom_initial_cycles -> uli-f-oom_initial_cycles (permission denied)
error: failed to push some refs to 'github.com:uli-f/cdk.git'

[johnmay]

Now I'm more confused how you did it @egonw. I followed this (https://tighten.com/insights/adding-commits-to-a-pull-request/) but had to rebase hence they are all in here now.

I will close and just do this seperately.

[egonw]

Now I'm more confused how you did it @egonw.

@johnmay, this is what I did (actually not in the way I hoped, but not sure if that feature still exists: in the past you could propose edits to patches):

1. the author of the PR has to have had enabled the option (checkbox in the "create PR" page). You then should see this (when the PR is still open; that's why I just had to reopen it again):

2. view the full file via the three dots in the top right:

3. check if the right branch version

4. then use the web UI to "edit" the file and push. because of the option in step 1, maintainers (you, me) should be able to push

[johnmay]

Ah OK, thanks. I did mange to push it as you saw but think I rebased wrongly. No worries.