-
Notifications
You must be signed in to change notification settings - Fork 5
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
Cmtj updates #77
Cmtj updates #77
Conversation
Reviewer's Guide by SourceryThis pull request implements several updates to the CMTJ simulator application, including UI improvements, new functionality for domain fitting, and changes to the application structure. The changes focus on enhancing user experience, adding new features, and improving code organization. File-Level Changes
Tips
|
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.
Hey @LemurPwned - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider implementing a more robust state management solution to reduce reliance on Streamlit's session state for complex data.
- The UI improvements and modularization are good. Consider further componentization to improve maintainability as the application grows.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -12,6 +12,25 @@ | |||
VSD_procedure) | |||
|
|||
|
|||
def create_single_domain(id_: str) -> Layer: |
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.
suggestion: Improve modularity of create_single_domain function
Consider refactoring create_single_domain() to accept parameters instead of reading directly from st.session_state. This would improve modularity, make the function more reusable, and easier to test.
def create_single_domain(id_: str, anisotropy_axis: str, **kwargs) -> Layer:
@@ -6,6 +6,20 @@ | |||
from cmtj.utils import Filters | |||
|
|||
|
|||
def read_mh_data(): |
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.
suggestion: Consider generalizing read_mh_data function
The read_mh_data() function is specific to a particular file format. Consider generalizing this function to handle various input formats, making it more flexible and reusable.
def read_data(file_format='mh'):
filedata = st.session_state.upload.read().decode("utf-8")
lines = filedata.split("\n")
if file_format == 'mh':
return parse_mh_data(lines)
elif file_format == 'other_format':
return parse_other_format(lines)
else:
raise ValueError(f"Unsupported file format: {file_format}")
@@ -0,0 +1,231 @@ | |||
from threading import RLock |
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.
suggestion: Consider modularizing the domain.py file
The domain.py file is quite long and handles multiple responsibilities. Consider splitting it into smaller, more focused components to improve maintainability and readability.
Summary by Sourcery
Enhance the CMTJ simulator Streamlit app by adding a domain fitting feature and a navigation system for better user experience. Refactor the app's parameter input sections for improved organization and update the Dockerfile to support the new app structure. Introduce a devcontainer configuration to streamline the development environment setup.
New Features:
Enhancements:
Build:
Deployment: