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

Api cleanup #15

Merged
merged 32 commits into from
Nov 28, 2023
Merged

Api cleanup #15

merged 32 commits into from
Nov 28, 2023

Conversation

calbaker
Copy link
Contributor

@calbaker calbaker commented Nov 3, 2023

  • clarifies process for creating SetSpeedTrainSim and SpeedLimitTrainSim
  • switches to using RailVehicle directly in place of RailVehicleMap in several places but not exhaustively
  • adds new yaml versions of rail vehicle files and tests them:
    def test_rail_vehicles():
    """
    Verifies that csv- and yaml-defined rail vehicles can be loaded and produce same result
    """
    rail_vehicle_path = alt.resources_root() / "rolling_stock"
    df = pd.read_csv(rail_vehicle_path / "rail_vehicles.csv")
    rail_vehicle_map = alt.import_rail_vehicles(str(rail_vehicle_path / "rail_vehicles.csv"))
    for i, row in df.iterrows():
    rv_csv = rail_vehicle_map[row['Car Type']]
    rv_yaml = alt.RailVehicle.from_file(
    str(rail_vehicle_path / f"{row['Car Type']}.yaml")
    )
    assert rv_csv.to_json() == rv_yaml.to_json()
  • various other changes to make API for constructing train simulations more clear

@calbaker calbaker marked this pull request as draft November 3, 2023 23:10
@calbaker calbaker self-assigned this Nov 3, 2023
changed `download_demo_files` to `copy_demo_files`
…y also `destination_id`

need a way to make it so that these are optional for this and probabbly mandator for speed limit train sim???
E     RuntimeError: [altrios-core/src/train/train_config.rs:363]
E     `origin_id`: "Funkytown" not found in `location_map` keys: ["Minneapolis", "Hibbing", "Allouez", "Superior"]
need time to asses whether exhaustive replacement makes sense
all tests but 1 pass
made use of `SHOW_PLOTS` more broadly
will merge this branch via PR and then fix in a separate branch
@calbaker calbaker marked this pull request as ready for review November 15, 2023 22:59
@SWRIganderson
Copy link
Collaborator

SWRIganderson commented Nov 16, 2023

@calbaker
I took a look at the examples in detail this evening. I think we should have more comments in the examples. I'm not saying you should do it though. This is on me too.

I also don't like the commented-out portions of the example that would be making plots. We should either make the plot or delete the code in my opinion.

I do like what you have done with the car yaml files instead of the csv. That seems a lot cleaner.

I have not looked at the rust changes in detail. I'll try to give it look on Thursday.

@calbaker
Copy link
Contributor Author

@sakhtar312 could you try to get these tests passing?

@sakhtar312
Copy link
Collaborator

@calbaker the tests are passing now. Installing the project in developer mode in tests.yaml file (adding -e flag to pip install .) did the trick.

Copy link
Collaborator

@sakhtar312 sakhtar312 left a comment

Choose a reason for hiding this comment

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

Reviewed the changes and both rust and python tests are passing. Approving the pull request.

@calbaker calbaker merged commit fafe01a into main Nov 28, 2023
2 checks passed
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