Skip to content

Conversation

@stellaprins
Copy link
Collaborator

@stellaprins stellaprins commented Jul 5, 2024

A first draft of the remote table.

May need some refactoring.

merge PR #37 first to avoid possible merging conflicts

@stellaprins
Copy link
Collaborator Author

Will have to tweak some things to make it compatible with pyNeuroML Biosimulations before it's ready for review.

See NeuroML/pyNeuroML#401 (comment)

@stellaprins stellaprins marked this pull request as ready for review August 6, 2024 07:37
@stellaprins
Copy link
Collaborator Author

Remote results has been added to the previous (local) results table as discussed in our meeting last week.

@stellaprins stellaprins requested a review from robertvi August 6, 2024 07:38
Copy link
Collaborator

@robertvi robertvi Aug 7, 2024

Choose a reason for hiding this comment

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

The "#!/usr/bin/env python" line has to be the first line in order to work. So it should either be removed or moved to the first line. I think the two python comments above and below it can be combined into one?

import utils
import os
import pandas as pd
from IPython.display import display_markdown
Copy link
Collaborator

Choose a reason for hiding this comment

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

display_markdown seems not to be needed in this script? Therefore we could probably remove the requirement for IPython?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Done! :)

import shutil
import argparse

parser = argparse.ArgumentParser(description='Test compatibility of different biosimulation engines')
Copy link
Collaborator

Choose a reason for hiding this comment

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

The --output-dir option is still being passed to the script in the non-oml.yml GH action, but is ignored due to this code block being removed. It might be useful to retain this option to allow easy testing without being forced to overwrite an existing output folder that you want to keep?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Done.

@stellaprins stellaprins merged commit de0c590 into development Aug 7, 2024
@stellaprins stellaprins deleted the feature/32-test-compatibility-biosimulators-remotely branch October 9, 2024 13:32
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.

3 participants