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

Fix/speed trace from slts in ssts #63

Merged
merged 45 commits into from
Sep 12, 2024
Merged

Conversation

calbaker
Copy link
Collaborator

@calbaker calbaker commented Apr 25, 2024

To verify that the loco_con and other objects are the same between the two simulations, you can:

  1. run pip install deepdiff
  2. run speed_limit_train_sim_demo.py interactively
  3. do something like loco_con_slts = loco_con
  4. run set_speed_train_sim_demo.py interactively in the same session
  5. run
from deepdiff import DeepDiff
import json
DeepDiff(
    json.loads(loco_con.to_json()),
    json.loads(loco_con_slts.to_json()),
)

and this will show that they're equal or specifically what's not equal if they're not. I have not checked everything exhaustively yet but am working on it.

You can now run debugging plots to produce:
image

  • double check that the above plot now shows line on line

This shows that the train resistance is different between the ssts and slts. I'm working on tracking this down.

@calbaker calbaker requested a review from SWRIganderson April 25, 2024 16:31
@calbaker
Copy link
Collaborator Author

I believe I found the culprit:
image

Generated in e02ccf0

@calbaker
Copy link
Collaborator Author

image

@calbaker
Copy link
Collaborator Author

self.state.pwr_whl_out = self.state.pwr_whl_out.max(-pwr_neg_max).min(pwr_pos_max);
is making me suspicious

@calbaker
Copy link
Collaborator Author

@SWRIganderson , seeing this line that Geordie wrote was my inspiration for the change. Also, I think we could prove that the two simulations produce identical results by making a variant for the traction-dependent component of the aux load so that this can be turned off during regen and/or dynamic braking, which should remove the only difference I know of.

@calbaker calbaker marked this pull request as draft June 27, 2024 18:37
@calbaker
Copy link
Collaborator Author

@SWRIganderson , this PR sure could use some love

calbaker and others added 2 commits August 9, 2024 16:23
…rom-slts-in-ssts

# Conflicts:
#	python/altrios/optimization/cal_and_val.py
#	rust/altrios-core/src/train/set_speed_train_sim.rs
…sim to help find difference with speed_limit_train_sim. Will repeat this exercise with other mode soon.
@SWRIganderson
Copy link
Collaborator

@calbaker I added a bunch of comments about set_speed_train_sim in commit 8c61cbc. I'll get the speed_limit)_train_sim knocked out soon. My biggest complaint is "res_net". We need change some nomenclature. res = reversible energy storage. Not resistance in my book. This is a minor nuisance in my book though.

@SWRIganderson
Copy link
Collaborator

@calbaker Check out commit 783039e. I have some comments in the speed limit train sim file. I am thinking that a key difference is that we pass speedtrace.dt in for a couple of arguments instead of self.state.dt. I am wondering if our states that we are using for energy calcs are off by one time step. Can we update the code to consistently use self.state.dt where ever possible. I am only half way through commenting the speed limit train sim file, but I feel like this could be something.

image

SWRIganderson and others added 5 commits August 27, 2024 08:18
…ltrios into fix/speed-trace-from-slts-in-ssts
speed_trace changed for some reason - not sure why
expected dataframe also needed to change, probably for same reason
self.loco_con.set_pwr_aux(Some(true))?;
//set the maximum power out based on dt. This is different than set speed train sim because that one calls the speed trace dt.
//why are we not passing in aux power here and self.state? Am I missing something obviouls here?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@SWRIganderson, as we have it now, aux load is determined entirely at the locomotive level and is solely dependent on a constant and tractive-power coefficient. As such, there's no need to pass this in. This obviously should be more sophisticated to account for other factors that impact aux load. I have lots of thoughts, but they all need vetting from another engineer, e.g. you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that we should have something more sophisticated. I think this needs to go down to the component/subsystem level so that we can capture differences between engine, battery, or traction motor aux loads. Not sure how we will validate against data though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@SWRIganderson , we've done some of this type of stuff in FASTSim. I am 98% sure I have multiple issues saying that we need better aux modeling so we'll get back to it when it becomes important.

self.train_res
.update_res(&mut self.state, &self.path_tpc, &Dir::Fwd)?;
//solve the required power. No argument is passed here unlike set_speed_train sim..
//I'm about to figure out why maybe......
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@SWRIganderson because the SpeedLimitTrainSim has a TimedLinkPath that contains a coarse speed trace, meaning that the train has to hit specified locations at specified times but that the exact speed is up to the train.


//this is similar to set_speed train sim, but speedtrace.dt is passed rather than state.dt
//could we get away with passing state.dt instead of speed_trace.dt in set_speed_train_sim?
//could this cause a lag of 1 dt in speed which would be a small but consistent offset in the calcs?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@SWRIganderson , hopefully https://github.com/NREL/altrios/pull/63/files#r1755701856 provides an explanation of why we would not want to do this.


//need for energy calc below. This is structured differently than set_speed_train_sim.
//However, I think the math is the same maybe.
//Would it be worth the effort to calculate this once when we initialize and save it as a parameter of the train? Then we would not have to calculate it every dt.
Copy link
Collaborator Author

@calbaker calbaker Sep 11, 2024

Choose a reason for hiding this comment

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

@SWRIganderson Probably not because it's a cheap calculation, and since I have aspirations of eventually incorporating our rust solver package to optionally have variable time step solvers, I would not want to had code this.

@calbaker calbaker marked this pull request as ready for review September 11, 2024 22:15
@calbaker calbaker force-pushed the fix/speed-trace-from-slts-in-ssts branch from 373e7d5 to 953d709 Compare September 12, 2024 16:46
@calbaker calbaker merged commit 49381bd into main Sep 12, 2024
4 checks passed
@calbaker calbaker deleted the fix/speed-trace-from-slts-in-ssts branch September 12, 2024 18:45
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.

2 participants