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

Better folder structure and file names #625

Merged
merged 27 commits into from Sep 10, 2019
Merged

Conversation

nathanlct
Copy link
Collaborator

@nathanlct nathanlct commented Jul 12, 2019

Pull request information

Description

This PR introduces major changes to file names and folder structures, as well as fo some class names, mainly for the following folders:

  • envs
  • multiagent_envs (now is a subfolder under envs)
  • scenarios
  • examples
  • removal of cooperative_merge (and loop_merge)

TODO:

  • better names for environments --> NL
  • merge multiagent_envs into envs --> AY
  • better name for scenarios so that they match the environments --> AY
  • better file names (e.g. sugiyama, loop,ring -> ring) --> AY, NL

This is how the new envs folder looks like (some classes names have also been changed):

envs
├── __init__.py
├── base.py
├── bay_bridge.py
├── bottleneck.py
├── merge.py
├── multiagent
│   ├── __init__.py
│   ├── base.py
│   ├── highway.py
│   ├── ring
│   │   ├── __init__.py
│   │   ├── accel.py
│   │   └── wave_attenuation.py
│   └── traffic_light_grid.py
├── ring
│   ├── __init__.py
│   ├── accel.py
│   ├── lane_change_accel.py
│   └── wave_attenuation.py
├── test.py
└── traffic_light_grid.py

This is how the new scenarios folder looks like (some classes names have also been changed):

scenarios/
├── __init__.py
├── base.py
├── bay_bridge.py
├── bay_bridge_toll.py
├── bottleneck.py
├── figure_eight.py
├── highway.py
├── merge.py
├── minicity.py
├── multi_ring.py
├── ring.py
└── traffic_light_grid.py

@AboudyKreidieh
Copy link
Member

This looks great. Should we also adjust for the names of the examples and scenarios in this PR?

@nathanlct
Copy link
Collaborator Author

@AboudyKreidieh Sure, I'll do that. Also the multiagent envs.

@ashkan-software
Copy link
Member

Thanks @nathanlct !

I think it's better if we wait a bit and discuss in the group meeting. I have discussed with @Yasharzf some good ideas about the names. Maybe we can incorporate them into these.

@nathanlct nathanlct changed the title Better names for environments Better folder structure and file names Jul 13, 2019
@nathanlct nathanlct added this to 0.5.0 -- the release to end them all in Release 0.4 -> 0.5 Jul 13, 2019
@ashkan-software
Copy link
Member

I am working on this. Please wait... :)

@kjang96
Copy link
Member

kjang96 commented Aug 12, 2019

Consider linking #600, continue talking about this next week

@ashkan-software
Copy link
Member

@AboudyKreidieh This is ready for your help now!
@nathanlct and me already took care of environments and scenarios. You can now start working on the Examples. :)
Just to remind you, you wanted to have something like:
python train.py "figure_eight" .....

@ashkan-software
Copy link
Member

Alright! All the checks are passed now. Can someone please review this? @AboudyKreidieh @eugenevinitsky?

Copy link
Member

@eugenevinitsky eugenevinitsky left a comment

Choose a reason for hiding this comment

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

This looks fine. I will approve once you merge in master and it looks good post the conflicts being resolved.

Copy link
Member

@eugenevinitsky eugenevinitsky left a comment

Choose a reason for hiding this comment

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

LGTM minus comments

Copy link
Member

@AboudyKreidieh AboudyKreidieh left a comment

Choose a reason for hiding this comment

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

One thing I'd like to see in this PR before we merge it is some deprecation warnings for old features, given these refactoring changes can be really cumbersome for users who have some code that they have written themselves. Stuff like

from flow.envs.base_env import Env

Yielding

flow.envs.base_env.Env will be deprecated in a future release, please use flow.envs.base.Env instead

with the codebase also returning the correct object. I tried to do that for some objects in flow/core/params.py and in PR #698

@AboudyKreidieh
Copy link
Member

also, this feature is for flow 0.5.0, so please hold off on merging

@ashkan-software
Copy link
Member

@AboudyKreidieh this is a great idea. Could you just hint me on which files or which imports I need to add this?

I think I need to add something like

                """Return network for this deprecated method."""
	        warnings.simplefilter('always', PendingDeprecationWarning)
	        warnings.warn(
	            "self.k.scenario will be deprecated in a future release. Please "
	            "use self.k.network instead.",
	            PendingDeprecationWarning
	        )

Am I right?

@AboudyKreidieh
Copy link
Member

@ashkan-software that's right. and in cases when they might be imported, add deprecations to the init.py files, as I also did in that PR

@AboudyKreidieh
Copy link
Member

@ashkan-software can you look into the deprecation procedure? One valuable resource might be looking into how tensorflow approaches this. maybe look into tensorflow/python/util/deprecation_wrapper.py

- added a deprecation function for classes and functions
- renames deprecation_warning ->deprecated_attribute
- added deprecation warnings for all old names of environments
- updated version to 0.5.0.dev
@AboudyKreidieh
Copy link
Member

I added a warning that looks like this:

__main__:1: PendingDeprecationWarning: The class flow.envs.green_wave_env.GreenWaveTestEnv is deprecated, use flow.envs.traffic_light_grid.TrafficLightGridTestEnv instead.

Copy link
Member

@ashkan-software ashkan-software left a comment

Choose a reason for hiding this comment

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

Thanks for the deprecation! This was supposed to be done by me, but I am sorry that I was super busy with my papers!
Anyways, the way you have done it is really smart!

Do we also need to make deprecated file (such as bottleneck_env) for the following files?

  • multiagent_env
  • bay_bridge.base
  • scenarios.grid
  • scenarios.loop

Everything else looks good.

Copy link
Member

@eugenevinitsky eugenevinitsky left a comment

Choose a reason for hiding this comment

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

LGTM. Lets never do this again haha.

@AboudyKreidieh AboudyKreidieh merged commit f4ed943 into master Sep 10, 2019
@AboudyKreidieh AboudyKreidieh deleted the better-file-names branch September 10, 2019 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Release 0.4 -> 0.5
0.5.0 -- the release to end them all ...
Development

Successfully merging this pull request may close these issues.

None yet

6 participants