Skip to content

PDF comparison #6

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

Closed
wants to merge 1 commit into from
Closed

PDF comparison #6

wants to merge 1 commit into from

Conversation

Sparks29032
Copy link
Contributor

Convert CIFs to PDFs and compare.

@Sparks29032 Sparks29032 marked this pull request as draft May 1, 2025 20:14
Copy link

github-actions bot commented May 1, 2025

Warning! No news item is found for this PR. If this is a user-facing change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.
For best practices, please visit
https://billingegroup.github.io/scikit-package/frequently-asked-questions.html#billinge-group-standards.

Copy link

codecov bot commented May 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.00%. Comparing base (807423a) to head (007f93f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main       #6   +/-   ##
=======================================
  Coverage   50.00%   50.00%           
=======================================
  Files           2        2           
  Lines          18       18           
=======================================
  Hits            9        9           
  Misses          9        9           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

this code looks pretty ugly. Can we work from the UCs and figure out what functionality we need and separate the functionality. These look like scripts not well architected code. They seem to mix up I/O and functionality and the functions are not well separated and doing only one thing. It is not even evident to me what is the purpose of this? WHich UC is being implementd?

@sbillinge
Copy link
Contributor

Thanks for pushing the PR early. Let's close this and revisit it in a more structured way.

@sbillinge sbillinge closed this May 2, 2025
@Sparks29032
Copy link
Contributor Author

This was the code we used for the poster, which you mentioned I should make a draft PR for. I have a lot of cleaning to do.

@sbillinge
Copy link
Contributor

I see, gotcha.

It's ok to leave it here for safe-keeping but make multiple new clean PRs for the different functionality and don't edit on this one.

We want general functions that handle different aspects ofthe UC that we will reuse across all the UCs, for example loading a single CIF to the ASE structure object (there should be an ASE function to do that that we can wrap), then doing the same thing for a set of CIFs, then different functions for building sets of CIFs (use glob with filters? Allow the user to specify a list? that but the list is in an input file on disc?) and so on. then our code can just operate on sets of structure objects etc. Clearly that functionality will be reused across all the different metrics. And so on.

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.

2 participants