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

Adding GeCo to Carla #154

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

Adding GeCo to Carla #154

wants to merge 6 commits into from

Conversation

GibbsG
Copy link

@GibbsG GibbsG commented Apr 26, 2022

Hi,

This PR is for adding GeCo, a counterfactual model, to Carla Benchmark. I also added the test in test/test_cfmodel.py and also updated the experiments to include GeCo.

Please have a look!

Thanks!
Gibbs

@JohanvandenHeuvel
Copy link
Collaborator

JohanvandenHeuvel commented Apr 26, 2022

Hi, thanks for submitting your PR.

Some comments on first glance:

  • Could you add ".DS_Store" to the .gitignore, and remove those files from the PR?
  • Did you follow this: https://carla-counterfactual-and-recourse-library.readthedocs.io/en/latest/installation.html#contributing? It seems there are some small problems with the formatting of the code. If there is anything unclear there please let us know, then we can add improvements.
  • Also I think Julia needs to be added to the requirements. Could you tell which version you use? (and if there are also other important packages, with their version) I'll try to fix that for you then, because there are multiple files with requirements so that's always a bit confusing.

Copy link
Collaborator

@indyfree indyfree left a comment

Choose a reason for hiding this comment

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

@GibbsG thanks for submitting your first PR! A great addition to also have some Julia based models included. See few comments below

experiments/run_experiment.py Outdated Show resolved Hide resolved
experimental_setup.yaml Show resolved Hide resolved
carla/recourse_methods/catalog/geco/model.py Show resolved Hide resolved
@GibbsG
Copy link
Author

GibbsG commented May 23, 2022

Hi, thanks for submitting your PR.

Some comments on first glance:

  • Could you add ".DS_Store" to the .gitignore, and remove those files from the PR?
  • Did you follow this: https://carla-counterfactual-and-recourse-library.readthedocs.io/en/latest/installation.html#contributing? It seems there are some small problems with the formatting of the code. If there is anything unclear there please let us know, then we can add improvements.
  • Also I think Julia needs to be added to the requirements. Could you tell which version you use? (and if there are also other important packages, with their version) I'll try to fix that for you then, because there are multiple files with requirements so that's always a bit confusing.

Hi @JohanvandenHeuvel thanks for reviewing it. I think most of them are addressed. The packages for GeCo itself are located at this file: carla/recourse_methods/catalog/geco/library/GeCo.jl/Manifest.toml. To run Julia in Python, we need one additional package from here https://pyjulia.readthedocs.io/en/latest/installation.html.

@GibbsG
Copy link
Author

GibbsG commented May 23, 2022

@GibbsG thanks for submitting your first PR! A great addition to also have some Julia based models included. See few comments below

Hi @indyfree, I really appreciate your comments. Please have a review again! Thanks.

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.

4 participants