Skip to content

Conversation

@AndrewSazonov
Copy link
Member

No description provided.

Copy link

@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.

This pull request does not contain a valid label. Please add one of the following labels: ['chore', 'fix', 'bugfix', 'bug', 'enhancement', 'feature', 'dependencies', 'documentation']

@AndrewSazonov AndrewSazonov added the chore Changes to the build process or auxiliary tools and libraries such as documentation generation label Oct 23, 2024
Copy link
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

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

Looks good. Minor issues raised for discussion

fail-fast: false
matrix:
os: [ubuntu-latest]
python-version: ['3.10', '3.11', '3.12']
Copy link
Member

Choose a reason for hiding this comment

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

I think we should concentrate on a single python version (3.12?) but include more OS-es. This is especially important for assuring file paths are properly set on Windows/Linux

Copy link
Member Author

Choose a reason for hiding this comment

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

Then let's have a full matrix with 3 python versions and 3 os versions to make sure everything is ok for all major combinations. Added to workflow config.


on:
push:
branches:
Copy link
Member

Choose a reason for hiding this comment

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

As discussed, this should also include on-dispatch case

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced the push with the workflow_dispatch.

parameters['values'].append(parameter.raw_value)
parameters['errors'].append(parameter.error)
parameters['units'].append(f'{parameter.unit:~H}')
parameters['units'].append(f'{parameter.unit:~P}')
Copy link
Member

Choose a reason for hiding this comment

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

at some point, we need to refactor out the plotting and printing methods from Job.py. This should be easily possible. (Not now, let's wait until you are done with the main functionality)

Copy link
Member Author

Choose a reason for hiding this comment

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

Completely agree with that.

except ImportError:
print("pandas not installed")

if 'JPY_PARENT_PID' in os.environ:
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be good to add a comment that this means 'if the job is run from the notebook', as it isn't obvious

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a method to determine if code is running in Jupyter notebook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Changes to the build process or auxiliary tools and libraries such as documentation generation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants