-
Notifications
You must be signed in to change notification settings - Fork 4
Update the main
with improvements done in development
branch since the last paper
#18
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
base: main
Are you sure you want to change the base?
Conversation
…sure correct results
…d some packages; conda-forge now
README updates from Main to Development
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR merges improvements from the development branch into main since the last paper publication. The changes primarily focus on configuration refactoring, enhanced callable generation, and improved computational capabilities.
Key changes include:
- Refactored YAML configuration structure from
callables
toelementalSpaces
- Added hybrid Scheil-equilibrium simulation capabilities
- Enhanced computational density settings and new utility functions
Reviewed Changes
Copilot reviewed 17 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
testing_input.yaml | Removed old test configuration file |
publication_input.yaml | Added comprehensive publication configuration with design spaces and stitching |
import_yaml.py | New utility for parsing YAML configurations and generating component mappings |
example_input.yaml | Added example configuration demonstrating multi-phase path planning |
ammap/templates/hybrid_scheil_eq_callable_template.py | New template for hybrid Scheil-equilibrium calculations |
ammap/templates/equilibrium_callable_template.py | Increased computational density from 2000 to 5000 |
ammap/callables/yaml_tools.py | New utility for updating feasible phases in YAML files |
ammap/callables/plotting.py | New 3D graph plotting functionality using igraph and plotly |
ammap/callableBuilders/hybrid_callables.py | New builder for hybrid callable generation |
ammap/callableBuilders/construct_callables_combo.py | Enhanced callable builder supporting multiple constraint types |
ammap/callableBuilders/construct_callables.py | Updated to use elementalSpaces instead of callables |
UPDATE.txt | Documentation of changes to implement |
Ti-Al-V-Cr.TDB | Added thermodynamic database for Ti-Al-V-Cr system |
Old_demos_and_workshops/AMMapDemonstration.ipynb | Updated execution counts and added error handling |
CONICRFE_input.yaml | New configuration for Co-Ni-Cr-Fe system analysis |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Path(f"{output_dir}/__init__.py").touch() | ||
|
||
# Read the hybrid Scheil-equilibrium template file | ||
with open('ammap/templates/hybrid_scheil_eq_callable_template_perp.py', 'r') as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file path 'hybrid_scheil_eq_callable_template_perp.py' does not match the actual template file 'hybrid_scheil_eq_callable_template.py' that was added to the repository. This will cause a FileNotFoundError.
with open('ammap/templates/hybrid_scheil_eq_callable_template_perp.py', 'r') as f: | |
with open('ammap/templates/hybrid_scheil_eq_callable_template.py', 'r') as f: |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import_yaml.py
Outdated
import re | ||
|
||
# Load the YAML file | ||
with open('example_input.yaml', 'r') as file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded filename 'example_input.yaml' should be parameterized to make the script reusable with different input files. Consider accepting the filename as a command-line argument or function parameter.
with open('example_input.yaml', 'r') as file: | |
import argparse | |
parser = argparse.ArgumentParser(description='Process a YAML file.') | |
parser.add_argument('filename', type=str, help='Path to the input YAML file') | |
args = parser.parse_args() | |
# Load the YAML file | |
with open(args.filename, 'r') as file: |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amr8004 Please have a look at my comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amr8004 This probably should land in some examples
or tutorial
directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should land in some examples
or tutorial
directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The publication demonstration can proably stay in root
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should land in some examples
or tutorial
directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renamig the folder to proper snake case with lower-case o
UPDATE.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need to delete this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstrings need an overhaul in terms of what it does. Examples should also move to the docstring
import_yaml.py
Outdated
# Combine all the names of each entry in designSpaces | ||
attainableSpaceComponents = [] | ||
for entry in design_spaces: | ||
components = re.findall(r'[A-Z][a-z]*', entry['name']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This belongs next to publication YAML for now, but probably should land in some examples
or tutorial
directory later
Path(f"{output_dir}/__init__.py").touch() | ||
|
||
# Read the hybrid Scheil-equilibrium template file | ||
with open('ammap/templates/hybrid_scheil_eq_callable_template_perp.py', 'r') as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the templates, you should change names of capitalized variables like 'Sfrac' or 'T_eq'. You can consider something like 'fracS' or 'eqT'. You can use capitalized constants |
Good progress overall 😃 Food for thought - we need to think about how to generalize populating the templates at some point, so that it is easy to do it with a substitution dictionary rather than hardcoding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is ready to be the main branch now
Hey @amr8004 - thanks for organizing everything 🚀 I had a quick look over the new structure and it generally looks good, but we need to still make it a bit more straightforward to approach by new users.
|
@amr8004 I will also run Copilot on this and it will probably give you a couple of items to address, since this PR is very sizable and changes half of the codebase, so I would be very surprised if there were no issues with some broken imports and deprecated references. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 22 out of 38 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
main
with improvements done in development
branch since the last papermain
with improvements done in development
branch since the last paper
A draft PR for @amr8004 to work on before we are ready to pull it and clean up the main afterwards