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

Implement batch_run #22

Merged
merged 10 commits into from
Dec 9, 2021
Merged

Implement batch_run #22

merged 10 commits into from
Dec 9, 2021

Conversation

xiki-tempula
Copy link
Contributor

FIx #15
The test is written against the qforce-example selimsami/qforce_examples#1

@xiki-tempula
Copy link
Contributor Author

@selimsami I think you have added some comments early this morning, but they are gone now.
I wonder if you deleted it accidentally or you prefer to do a better review later?

@selimsami
Copy link
Owner

Yes I saw a problem somewhere but then I realized my solution was not good.

I'll have another look soon.

Copy link
Owner

@selimsami selimsami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, I have some minor comments, it can be merged after they are implemented.

@@ -357,26 +369,31 @@ def check_for_qm_data(self, job, config, mol, qm):

else:
self.check_new_scan_data(job, mol, config, qm)
self.write_have_or_missing(job)
self.write_have_or_missing(job, config)
nx.write_gpickle(self.graph, f"{self.dir}/identifier_{self.hash_idx}")
self.write_xyz()
with open(f"{self.dir}/qm_method_{self.hash_idx}", 'w') as file:
json.dump(self.graph.graph['qm_method'], file, sort_keys=True, indent=4)

if not self.has_data:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not (self.has_data or (config.batch_run and self.has_inp)):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't quite understand this part.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that was a bit ambiguous. I meant replace:

            if not self.has_data:
                self.make_qm_input(job, qm)
                if not (config.batch_run and self.has_inp):
                    self.make_qm_input(job, qm)

with:

            if not (self.has_data or (config.batch_run and self.has_inp)):
                self.make_qm_input(job, qm)

if self.has_data:
status = 'have'
else:
status = 'missing'

if config.batch_run and self.has_inp:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be an elif before the else above


check_and_notify(job, config.scan, len(unique_dihedrals), len(fragments))
check_and_notify(job, config.scan, len(unique_dihedrals), len(fragments),
len(generated))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too important but: I use line length of max 100, would be good to be consistent with that. (might apply elsewhere also)

for name, atomids in unique_dihedrals.items():
frag = Fragment(job, config, mol, qm, atomids, name)
if frag.has_data:
fragments.append(frag)
if frag.has_inp:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elif config.batch_run and frag.has_inp:

Copy link
Contributor Author

@xiki-tempula xiki-tempula Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config here is the fragment config and doesn't have the batch_run attribute.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think it's the general config. Then you need: config.scan.batch_run

@@ -60,6 +63,8 @@ def check_and_notify(job, config, n_unique, n_have):
print("All scan data is available. Fitting the dihedrals...\n")
else:
print(f"{n_missing} of them are missing the scan data.")
if config.batch_run:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if n_generated > 0: 
    print(f"{n_generated} of them generated previously (Batch run enabled).")
if n_missing - n_generated > 0:
    print(f"QM input files for them are created in: {job.frag_dir}")

@xiki-tempula
Copy link
Contributor Author

@selimsami Thanks for the review. I have addressed the points that I can address.

@xiki-tempula
Copy link
Contributor Author

@selimsami Thanks for the review, all of the points have been addressed.

@codecov
Copy link

codecov bot commented Dec 9, 2021

Codecov Report

Merging #22 (e709ef1) into master (4bb81bb) will increase coverage by 0.60%.
The diff coverage is 58.97%.

❗ Current head e709ef1 differs from pull request most recent head b842f9b. Consider uploading reports for the commit b842f9b to get more accurate results

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2021

Codecov Report

Merging #22 (d4bc810) into master (4bb81bb) will decrease coverage by 3.49%.
The diff coverage is 0.00%.

@selimsami selimsami merged commit b8c0fdb into selimsami:master Dec 9, 2021
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.

Support for large scale screening
3 participants