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

Implement Hype forcing & model #308

Merged
merged 24 commits into from
Jun 20, 2022
Merged

Implement Hype forcing & model #308

merged 24 commits into from
Jun 20, 2022

Conversation

sverhoeven
Copy link
Member

@sverhoeven sverhoeven commented May 31, 2022

TODO

  • HypeForcing
  • Hype model
  • In docs
  • Check docs/adding_models.rst steps have been done
  • Add https://sourceforge.net/projects/hype/files/release_hype_5_6_2/demo.zip as example parameter set
  • use model.time_as_isostr, but model.bmi.get_time_units() returns invalid 'hours since start of simulation'
  • find demo set where spatial position is known of sub-basins and then use model.get_value_as_xarray()
  • Fix for Python 3.7 -> lets drop 3.7 support
  • Copy sif to dcache

@sverhoeven sverhoeven changed the title Implement Hype forcing Implement Hype forcing & model May 31, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@SarahAlidoost SarahAlidoost self-requested a review June 1, 2022 14:51
@SarahAlidoost
Copy link
Contributor

SarahAlidoost commented Jun 2, 2022

@sverhoeven the notebook hype.ipynb using docker gives the error:

---------------------------------------------------------------------------
DeadContainerException                    Traceback (most recent call last)
Input In [6], in <cell line: 1>()
----> 1 cfg_file, cfg_dir = model.setup()
      2 cfg_file, cfg_dir

File ~/GitHub/ewatercycle/src/ewatercycle/models/hype.py:152, in Hype.setup(self, start_time, end_time, crit_time, cfg_dir)
    150 # start container
    151 work_dir = str(cfg_dir_as_path)
--> 152 self.bmi = _start_container(self.version, work_dir)
    154 return str(cfg_file), work_dir

File ~/GitHub/ewatercycle/src/ewatercycle/models/hype.py:217, in _start_container(version, work_dir)
    215 elif CFG["container_engine"].lower() == "docker":
    216     image = _version_images[version]["docker"]
--> 217     return BmiClientDocker(
    218         image=image,
    219         image_port=55555,  # TODO needed?
    220         work_dir=work_dir,
    221     )
    222 else:
    223     raise ValueError(
    224         f"Unknown container technology in CFG: {CFG['container_engine']}"
    225     )

File ~/mambaforge/envs/ewatercycle/lib/python3.10/site-packages/grpc4bmi/bmi_client_docker.py:88, in BmiClientDocker.__init__(self, image, work_dir, image_port, host, input_dirs, user, remove, delay, timeout)
     86         logs = self.logs()
     87         msg = f'Failed to start Docker container with image {image}, Container log: {logs}'
---> 88         raise DeadContainerException(msg, exitcode, logs)
     90 super(BmiClientDocker, self).__init__(BmiClient.create_grpc_channel(port=port, host=host), timeout=timeout)

DeadContainerException: Failed to start Docker container with image ewatercycle/hype-grpc4bmi:feb2021, Container log: terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_M_construct null not valid

I submitted issue eWaterCycle/ewatercycle-hype#5


forcing_file = Path(forcing_path).name
Copy link
Contributor

Choose a reason for hiding this comment

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

the from pathlib import Path at the top of the module can be removed. Shall we also add Hype generate forcing (e.g. the code block below) to the notebook generate_forcing.ipynb?

# Hype
hype_forcing = ewatercycle.forcing.generate(
    target_model="hype",
    dataset="ERA5",
    start_time="1990-01-01T00:00:00Z",
    end_time="1990-12-31T00:00:00Z",
    shape="./data/Rhine/Rhine.shp",
)
print(hype_forcing)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, including the print output

"outputs": [],
"source": [
"parameter_set_dir = \"<path where demo.zip was extracted to>\"\n",
"parameter_set_dir = \"/tmp/o/demo\"\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"parameter_set_dir = \"/tmp/o/demo\"\n",

Copy link
Contributor

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@sverhoeven I tested the notebook on Snellius. It works. I left some minor comments. We need to add "Hype" to example.rst as well. Similar to other notebooks, we may run the model for a short time period and plot the time series in the notebook hype.ipynb.

@sverhoeven
Copy link
Member Author

@sverhoeven I tested the notebook on Snellius. It works. I left some minor comments. We need to add "Hype" to example.rst as well. Similar to other notebooks, we may run the model for a short time period and plot the time series in the notebook hype.ipynb.

Added hype to examples.rst and discharge is plotted now.

@sonarcloud
Copy link

sonarcloud bot commented Jun 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

88.8% 88.8% Coverage
0.0% 0.0% Duplication

@SarahAlidoost
Copy link
Contributor

esmvaltool issue ESMValGroup/ESMValTool#2678

@sverhoeven
Copy link
Member Author

thanks for reviewing. I spawned some additional issues eWaterCycle/ewatercycle-hype#6, eWaterCycle/ewatercycle-hype#4

@sverhoeven sverhoeven merged commit fa82136 into main Jun 20, 2022
@sverhoeven sverhoeven deleted the hype-forcing branch June 20, 2022 09:01
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