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

Improves folder's names on multiple coformers runs #68

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

MilitaoLucas
Copy link
Contributor

This makes it safer to add new coformers mid run and re-generate a report for new coformers

…I think that it is safer this way as it doesn't depend on alphabetical order.
Copy link

sonarcloud bot commented Aug 22, 2024

@MilitaoLucas MilitaoLucas changed the title I meant to do it a while ago but forgot. Improves folders name's on multiple coformers runs Aug 22, 2024
@MilitaoLucas MilitaoLucas changed the title Improves folders name's on multiple coformers runs Improves folder's names on multiple coformers runs Aug 22, 2024
@pmbulit
Copy link
Member

pmbulit commented Aug 23, 2024

@MilitaoLucas I like the idea, but this doesn't change the script's behaviour for me. Once it enters the loop in l:345 the list doesn't get updated even if I add extra coformer files.

@MilitaoLucas
Copy link
Contributor Author

MilitaoLucas commented Aug 23, 2024

@MilitaoLucas I like the idea, but this doesn't change the script's behaviour for me. Once it enters the loop in l:345 the list doesn't get updated even if I add extra coformer files.

Oh, I think I expressed myself badly. The list isn't exactly supposed to increase at the run. The loop could verify if there are additional runs at each interaction and add it to the list. It would be better this way. I kind of focused on my use case and was not generic enough.

@pmbulit pmbulit self-requested a review August 23, 2024 15:52
@pmbulit
Copy link
Member

pmbulit commented Aug 23, 2024

Got it. Is the idea that you re-run it only on the new ones? I think we could modify the function that makes the report so it reads the json files instead of saving the results to memory. That would also make re-running the script after a crash a bit friendlier

@MilitaoLucas
Copy link
Contributor Author

Got it. Is the idea that you re-run it only on the new ones? I think we could modify the function that makes the report so it reads the json files instead of saving the results to memory. That would also make re-running the script after a crash a bit friendlier

What I did in previous PRs makes it so the results for MCHBP scores are stored in success.json. So it already handles crashes perfectly. Now for adding coformers mid run, for loops are impractical.
The enumerator that is created doesn't comport new elements or behaves when adding or removing elements. What I could do is add a while loop that verifies if the new coformer list has new elements. That way, at each iteration, we could compare the new list with the initial list and mark as completed every element that was already run. I could also delete coformers that are removed from the library easily. But I don't think this is very good behaviour.

@pmbulit
Copy link
Member

pmbulit commented Aug 23, 2024

I was thinking that adding a flag to not run if the folder for the coformer already exists and has data and another flag called --report_only or something like that could be good for people less confident with scripting, That way if it crashes half way or you want to add just a few more coformers you can still generate a report including all the data you already have.

@MilitaoLucas
Copy link
Contributor Author

The former is default behaviour. Do you want to leave a negative flag, or change the default behaviour to not ignore already ran folders? I think it is better as default behaviour.
The later I can easily do, as I already did something like it for my fork.

@pmbulit
Copy link
Member

pmbulit commented Aug 23, 2024

That shows you how often I re-run the script 😅 The default is fine. Would you prefer I approve this PR and you make another one later or make additional commits here?

@MilitaoLucas
Copy link
Contributor Author

That shows you how often I re-run the script 😅 The default is fine. Would you prefer I approve this PR and you make another one later or make additional commits here?

Both work fine for me. But I guess you are in the end of your work day, right? Cambridge is at 0 GMT. So I think it would be better to include it here as it works, and I create another one this weekend to finalize the other aspects.

@pmbulit
Copy link
Member

pmbulit commented Aug 23, 2024

Sounds good. I'll have a loot at this next week then.

@MilitaoLucas
Copy link
Contributor Author

MilitaoLucas commented Aug 28, 2024

I had some stuff coming up this week. Unfortunately, I will not be able to finish this until next week. I think this could be merged, but if you want to keep it open until then, that is also fine. I am sorry for this problem.

@pmbulit pmbulit merged commit 4929a81 into ccdc-opensource:main Aug 28, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants