UDST / orca

Python library for task orchestration
https://udst.github.io/orca/
BSD 3-Clause "New" or "Revised" License
53 stars 21 forks source link

Rework hdf output #11

Closed bridwell closed 8 years ago

bridwell commented 8 years ago

This addresses #9 and supersedes a previous pull request I made https://github.com/UDST/orca/pull/10:

orca.run(
    ['my_step'],
    range(2010, 2030),
    data_out=out_file,
    out_interval=5,
    out_base_tables=orca.list_tables(),
    out_run_tables=['a_table']
)
# just output a couple tables
orca.write_tables(out_h5, ['buildings', 'persons'])

# write out everything that's registered
orca.write_tables(out_h5)

# still allow for step inference
step_tables = orca.get_step_table_names(['my_step'])
orca.write_tables(out_h5, step_tables)

The docstrings and tests have been updated to reflect these changes.

I looked through the repo and the only method calling write_tables is the run method. So all tests pass fine. However, if there are calls to orca.write_tables coming from outside orca, then these will now fail. I don't see why this previously would be called outside the context of a run, but if that is the case then I can revert the signature back to the previous version.

coveralls commented 8 years ago

Coverage Status

Changes Unknown when pulling ef03a5d6b3043b8f16af4011f4169da9a0d71cee on AZMAG:rework_hdf_output into \ on UDST:master**.

bridwell commented 8 years ago

This passes for python 3.3 but fails for 2.7 and 3.4 on an assert column series equal.

It appears that setting values via .loc is changing the data type from int64 to float. Should the update_col_from_series method be updated so that the data type of the incoming series is cast to match the local column?

Or should update_col_from_series check the data types of the series and the incoming values and raise an exception if they don't match?

coveralls commented 8 years ago

Coverage Status

Changes Unknown when pulling fa560ee7f2911395cdf4e129699c4980b7a518f3 on AZMAG:rework_hdf_output into \ on UDST:master**.

fscottfoti commented 8 years ago

FWIW these changes all seem very reasonable to me. Ready to merge?

coveralls commented 8 years ago

Coverage Status

Changes Unknown when pulling 7aba30933a17b68694c86634a8961dd15839f4f9 on AZMAG:rework_hdf_output into \ on UDST:master**.

bridwell commented 8 years ago

OK I think it's ready now.

janowicz commented 8 years ago

These changes make a lot of sense to me too. Looks good.