Skip to content

Conversation

@jaredthomas68
Copy link
Contributor

The core glue code seems to work. However, the way the wisdom wrapper was set up was incompatible with the glue code. This was an initial attempt to get working.

  • initial wisdom wrap relies on latents setting after set up, but the structure of the OpenMDAO problem had to be hard coded
  • ideally we would not hard code the structure for wisdom inputs, but set them directly. However, we can't do that if our classes just inherit from wisdom

@jaredthomas68 jaredthomas68 requested a review from cfrontin June 9, 2025 22:46
@jaredthomas68 jaredthomas68 changed the base branch from main to develop June 10, 2025 16:22
@jaredthomas68 jaredthomas68 requested a review from Copilot June 10, 2025 16:23

This comment was marked as outdated.

@cfrontin cfrontin linked an issue Jun 17, 2025 that may be closed by this pull request
@cfrontin cfrontin marked this pull request as ready for review June 23, 2025 22:01
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this right? an empty yaml file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

empty file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

empty file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this looks great!

Copy link
Collaborator

Choose a reason for hiding this comment

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

empty file?

- ["AEP_farm", "financese.plant_aep_in"]
- ["landbosse.total_capex_kW", "financese.bos_per_kW"]
systems:
layout2aep:
Copy link
Collaborator

Choose a reason for hiding this comment

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

approximate subtotals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

import ard.glue.prototype as glue

# import ard.api.prototype as glue
import ard.api.interface as glue2
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this permanent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +79 to +114
# # plot convergence
# ## read cases
# cr = om.CaseReader(
# prob.get_outputs_dir()
# / input_dict["analysis_options"]["recorder"]["filepath"]
# )
#
# # Extract the driver cases
# cases = cr.get_cases("driver")
#
# # Initialize lists to store iteration data
# iterations = []
# objective_values = []
#
# # Loop through the cases and extract iteration number and objective value
# for i, case in enumerate(cases):
# iterations.append(i)
# objective_values.append(
# case.get_objectives()[
# input_dict["analysis_options"]["objective"]["name"]
# ]
# )
#
# # Plot the convergence
# plt.figure(figsize=(8, 6))
# plt.plot(iterations, objective_values, marker="o", label="Objective (LCOE)")
# plt.xlabel("Iteration")
# plt.ylabel("Objective Value (Total Cable Length (m))")
# plt.title("Convergence Plot")
# plt.legend()
# plt.grid()
# plt.show()
#
# optiwindnet.plotting.gplot(prob.model.optiwindnet_coll.graph)
#
# plt.show()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have this in the analysis script now, so we can probably remove it from here, I think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think @jaredthomas68

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am inclined to leave it. Since this give users trying the examples some outputs to work with. We should consider creating some better viz tools so we don't need so much code though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think we should just put the rest of optimization_demo.py into the Jupyter notebook and blow away the .py file. It is confusing to have to run. multiple files for a single example. I'd rather have everything in Jupyter unless there is a run time concern or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be a new PR though since this PR is already pretty bloated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh yeah, that's a good idea. maybe all of these examples should be the same way? let's commit as-is for now and create a new PR to clean the examples to all use notebooks

This comment was marked as outdated.

jaredthomas68 and others added 5 commits July 16, 2025 09:09
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Collaborator

@cfrontin cfrontin left a comment

Choose a reason for hiding this comment

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

Let's get this merged conditioned on a new PR to fix up the examples for a consistent notebook format.

@cfrontin
Copy link
Collaborator

I brought in the optiwindnet changes. I don't think they should break anything, but eyeball the changes I made to rectify those!

@jaredthomas68 jaredthomas68 requested a review from Copilot July 17, 2025 15:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the test suite to align with the new glue/API code, expanding and standardizing how modeling options are passed into tests across multiple modules. Key changes include:

  • Augmenting modeling_options in many tests to explicitly set layout and orientation parameters (spacing_primary, spacing_secondary, angle_orientation, angle_skew, x_turbines, y_turbines, x_substations, y_substations).
  • Migrating from legacy ard.glue.prototype functions to the new set_up_ard_model API and removing obsolete imports.
  • Refactoring test setup to reduce duplication via helper functions (make_modeling_options) and adopting subtests in pyrite file validations.

Reviewed Changes

Copilot reviewed 80 out of 90 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/unit/utils/test_io.py Added nested-list/dict tests for replace_key_value.
test/unit/ard/layout/test_spacing.py Removed unused JAX imports.
test/unit/ard/layout/test_gridfarm.py Extended modeling_options with spacing and angle keys.
test/unit/ard/farm_aero/test_templates.py Refactored wind query instantiation and integrated into modeling_options.
test/unit/ard/farm_aero/test_floris.py Updated pyrite validation to use subtests; now imports pytest.
test/unit/ard/cost/test_wisdem_wrap.py Switched to load-only pyrite validation and added subtests.
test/unit/ard/collection/test_templates.py Added coordinate arrays for turbines and substations.
test/unit/ard/collection/test_optiwindnet.py Introduced make_modeling_options, restructured OpenMDAO setup.
test/unit/ard/api/test_interface_unit.py New unit tests for set_up_ard_model error handling.
test/system/ard/* Updated system tests to use set_up_ard_model, include new params, and supply input files under inputs_*.
Comments suppressed due to low confidence (3)

test/unit/ard/farm_aero/test_floris.py:231

  • Missing import for Path from pathlib. Add from pathlib import Path at the top of the file so that Path(...) resolves correctly.
            Path(__file__).parent / "test_floris_aep_pyrite.npz",

test/unit/ard/cost/test_wisdem_wrap.py:171

  • Missing import for numpy as np at the top of the file. Add import numpy as np so that np.isclose is available.
                assert np.isclose(value, pyrite_data[key], rtol=5e-3), (

test/unit/ard/cost/test_wisdem_wrap.py:8

  • Missing import for Path from pathlib. Add from pathlib import Path so that file-path operations work as intended.
import ard.utils.test_utils

@jaredthomas68 jaredthomas68 requested a review from cfrontin July 17, 2025 15:13
@cfrontin
Copy link
Collaborator

i'm coming in super late here, but should we add a surrogate entry at the group level to the glue from the jump? this might be unnecessary but there's a potential future-proofing we could have here

@jaredthomas68
Copy link
Contributor Author

i'm coming in super late here, but should we add a surrogate entry at the group level to the glue from the jump? this might be unnecessary but there's a potential future-proofing we could have here

i'm coming in super late here, but should we add a surrogate entry at the group level to the glue from the jump? this might be unnecessary but there's a potential future-proofing we could have here

I think this is for later. I created an issue for it #111

@jaredthomas68 jaredthomas68 merged commit 6890791 into NLRWindSystems:develop Jul 17, 2025
7 checks passed
@jaredthomas68 jaredthomas68 deleted the gluedraft branch July 17, 2025 16:10
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.

Iterate glue code

2 participants