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 temperature-aware operational domain simulation. #646

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

Conversation

Drewniok
Copy link
Collaborator

Description

This PR expands the operational domain simulation to include temperature. It is now possible to simulate temperature at each parameter point, creating a temperature-aware operational domain.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have added a changelog entry.
  • I have created/adjusted the Python bindings for any new or updated functionality.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@Drewniok Drewniok changed the title ✨ Added temperature-aware operational domain. ✨ Added temperature-aware operational domain simulation. Jan 25, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Copy link

codecov bot commented Jan 25, 2025

Codecov Report

Attention: Patch coverage is 95.56314% with 26 lines in your changes missing coverage. Please review.

Project coverage is 98.37%. Comparing base (a811148) to head (503dbdf).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../algorithms/simulation/sidb/operational_domain.hpp 90.57% 13 Missing ⚠️
test/io/write_operational_domain.cpp 95.58% 6 Missing ⚠️
include/fiction/io/write_operational_domain.hpp 88.88% 4 Missing ⚠️
.../algorithms/simulation/sidb/operational_domain.cpp 98.46% 2 Missing ⚠️
...st/algorithms/simulation/sidb/time_to_solution.cpp 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #646      +/-   ##
==========================================
- Coverage   98.41%   98.37%   -0.04%     
==========================================
  Files         252      254       +2     
  Lines       40856    41113     +257     
  Branches     1863     1872       +9     
==========================================
+ Hits        40208    40446     +238     
- Misses        648      667      +19     
Files with missing lines Coverage Δ
...clude/fiction/algorithms/physical_design/exact.hpp 89.62% <ø> (ø)
...imulation/sidb/calculate_energy_and_state_type.hpp 100.00% <ø> (ø)
...hms/simulation/sidb/can_positive_charges_occur.hpp 100.00% <100.00%> (ø)
...ion/algorithms/simulation/sidb/clustercomplete.hpp 99.27% <100.00%> (ø)
...lgorithms/simulation/sidb/critical_temperature.hpp 96.57% <100.00%> (ø)
...on/algorithms/simulation/sidb/defect_clearance.hpp 100.00% <100.00%> (ø)
...on/algorithms/simulation/sidb/defect_influence.hpp 96.51% <100.00%> (-0.05%) ⬇️
...algorithms/simulation/sidb/energy_distribution.hpp 100.00% <100.00%> (ø)
.../sidb/equivalence_check_for_simulation_results.hpp 100.00% <100.00%> (ø)
.../algorithms/simulation/sidb/ground_state_space.hpp 100.00% <100.00%> (ø)
... and 28 more

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@@ -173,3 +200,148 @@
}
}
}

TEST_CASE("Write operational domain with floating-point parameter and temperature values", "[write-operational-domain]")

Check warning

Code scanning / CodeQL

Poorly documented large function Warning test

Poorly documented function: fewer than 2% comments for a function of 144 lines.
@Drewniok Drewniok requested a review from marcelwa February 3, 2025 07:50
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

# Conflicts:
#	experiments/equivalence_checking_exact_simulation/equivalence_checking_exact_simulation.cpp
Copy link
Collaborator

@marcelwa marcelwa left a comment

Choose a reason for hiding this comment

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

Thank you very much for the restructuring. I think it's already so much cleaner. I spotted some locations where I'd like to offer feedback to hopefully improve upon it even further. Hopefully, this is helpful to you.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@Drewniok Drewniok requested a review from marcelwa February 7, 2025 16:05
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Copy link
Collaborator

@marcelwa marcelwa left a comment

Choose a reason for hiding this comment

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

Thanks a lot for addressing my comments so swiftly. The code is a lot cleaner already. Well done! I think, after another iteration, we should be ready to merge 🙂

@@ -37,21 +37,20 @@ inline void physically_valid_parameters(pybind11::module& m)
{
namespace py = pybind11;

py::class_<fiction::operational_domain<fiction::parameter_point, uint64_t>>(m, "physically_valid_parameters_domain")
py::class_<fiction::sidb_simulation_domain<fiction::parameter_point, uint64_t>>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing docstrings for the class and member functions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that the docstring is not generated. Do you have an idea why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately not. I have observed this behavior a few times myself as well. It's really mysterious to me 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, can we merge it anyway?

cli/cmd/simulation/opdom.hpp Outdated Show resolved Hide resolved
@Drewniok
Copy link
Collaborator Author

Drewniok commented Feb 9, 2025

Thanks a lot for addressing my comments so swiftly. The code is a lot cleaner already. Well done! I think, after another iteration, we should be ready to merge 🙂

Thank you so much for your quick and extensive support!

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@Drewniok Drewniok requested a review from marcelwa February 10, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants