WIP: Add new wind and solar clusters to GenX output

Work in progress

[x] Select clusters based on user constraints
[x] Get cluster hourly profiles
[ ] Get existing wind/solar hourly profiles for each IPM region

[ ] Create single profile for each model region, weighted avg of individual profiles based on capacity (Existing_Cap_MW) in each IPM region (new code)

[x] Get cluster transmission distances and max capacity

[x] Also need to get non-wind/solar transmission distances from somewhere (csv or settings file)

[x] Add transmission ("tx") distances and max capacity to generators DataFrame (single resource per row)
[x] Calculate capital expenditure ("capex") and annuity based on transmission distances
[x] Update resource annuity
[x] Create generator variability file (one column per resource, 8784 rows)

[x] Add option to drop 2012-02-29? (8760 rows)

Thanks for starting this PR and including fixes to the README, genx_settings.yml, etc.

@gschivley Here is a beta MWE with a trivial dataset. It serves to illustrate the data format I have in mind (and an alternative, commented out and not implemented).
Deviations from your example file:

Make cluster_level an integer
Add cluster id in order of aggregation
Add parent id (parent_id) for tree traversal
Make all variables lowercase snake case (no exceptions!)

import pandas as pd

# No duplication of tree nodes
# (smaller, unique ids, easier to traverse but must do so to build cluster levels)
df = pd.DataFrame(
    {
        "ipm_region": "WEC_CALN",
        "cbsa_id": 1,
        # Only base cluster_level equal to number of rows
        "cluster_level": [3, 3, 3, 2, 1],
        # Order cluster ids by order of aggregation
        "id": [1, 2, 3, 4, 5],
        "parent_id": [4, 4, 5, 5, 5],
        "gw": [1, 1, 1, 2, 3],
        "lcoe": [0.2, 0.3, 0.4, 0.25, 0.3],
    }
)

# Duplication of tree nodes
# (larger, non-unique ids, harder to traverse, easy to extract cluster levels)
# df = pd.DataFrame(
#     {
#         "ipm_region": "WEC_CALN",
#         "cbsa_id": 1,
#         # All cluster_level equal to number of rows
#         "cluster_level": [3, 3, 3, 2, 2, 1],
#         # Repeat ids to indicate
#         "id": [1, 2, 3, 4, 3, 5],
#         "parent_id": [4, 4, 3, 5, 5, 5],
#         "gw": [1, 1, 1, 2, 1, 3],
#         "lcoe": [0.2, 0.3, 0.4, 0.25, 0.4, 0.3],
#     }
# )

def get_base_clusters(df, region):
    # settings['region_aggregations']
    ipm_regions = {
        "CA_N": ["WEC_CALN", "WEC_BANC"],
        "CA_S": ["WEC_SCE", "WEC_LADW", "WECC_SCE", "WEC_SDGE", "WECC_IID"],
    }[region]
    return (
        df[df["ipm_region"].isin(ipm_regions)]
        .groupby(["ipm_region", "cbsa_id"])
        .apply(lambda g: g[g["cluster_level"] == g["cluster_level"].max()])
        .reset_index(drop=True)
    )

def aggregate_clusters(df, region, min_capacity=None, max_clusters=np.inf):
    # settings['region_aggregations']
    cdf = get_base_clusters(df, region).sort_values("lcoe")
    if min_capacity:
        # Drop clusters with highest LCOE until min_capacity reached
        end = cdf["gw"].cumsum().searchsorted(min_capacity) + 1
        if end > len(cdf):
            raise ValueError(
                f"Capacity in {region} ({cdf['gw'].sum()} GW) less than minimum ({min_capacity} GW)"
            )
        else:
            cdf = cdf[:end]
    while len(cdf) > max_clusters:
        # Find all parents with both children among clusters
        child_counts = cdf["parent_id"].value_counts()
        parent_ids = child_counts[child_counts == 2].index
        if parent_ids.empty:
            break
        # Find parent with lowest LCOE
        if len(parent_ids) > 1:
            # NOTE: Assumes unique ids
            is_parent = df["id"].isin(parent_ids)
            best_parent_id = df[is_parent].sort_values("lcoe")["id"].iloc[0]
        else:
            best_parent_id = parent_ids[0]
        # Remove children of parent with lowest LCOE
        cdf = cdf[cdf["parent_id"] != best_parent_id]
        # Add parent with lowest LCOE
        # NOTE: Slow. Consider using dict of dict indexed by id.
        cdf.loc[len(cdf)] = df[df["id"] == best_parent_id].iloc[0]
    return cdf

aggregate_clusters(df, "CA_N", min_capacity=2, max_clusters=2)
aggregate_clusters(df, "CA_N", min_capacity=3, max_clusters=3)
aggregate_clusters(df, "CA_N", min_capacity=2, max_clusters=3)
aggregate_clusters(df, "CA_N", min_capacity=3, max_clusters=3)
aggregate_clusters(df, "CA_N", min_capacity=3, max_clusters=2)
aggregate_clusters(df, "CA_N", min_capacity=3, max_clusters=1)

@gschivley The suggestion of promoting single children to their parent in the pre-computed tree means essentially creating a new tree. It is not sufficient to take the weighted mean of the original parent.
For example, assume three base nodes:

a = capacity: 2, lcoe: 2
b = capacity: 2, lcoe: 4
c = capacity: 2, lcoe: 6

and the following parents:

ab = a + b = capacity: 4 (2 + 2), lcoe: 3 (2/4 * 2 + 2/4 * 4)
abc = ab + c = capacity: 6 (4 + 2), lcoe: 4 (4/6 * 3 + 2/6 * 6)

Say only a and b are selected. If we weigh abc based on the sum of the capacity of its selected children (a and b), we get an incorrect result for mean attributes like lcoe: 8/3 (4/6 * 4). Rather, the correct calculation is to promote ab to abc's position (lcoe: 3). Further upstream, ab could combine with other nodes, forming novel combinations of clusters despite following paths prescribed by the original tree. In other words, all aggregated cluster attributes (generation profiles, lcoe, ...) need to be calculated from the attributes of the base clusters.
Do you still prefer this over the aggregation of base clusters using a simple rule for within each metro area (e.g. nearest pairwise lcoe or lowest parent lcoe)? Checking in before I get any deeper in the weeds, and to make sure I understood your original suggestion.

Say only a and b are selected. If we weigh abc based on the sum of the capacity of its selected children (a and b), we get an incorrect result for mean attributes like lcoe: 8/3 (4/6 * 4).

I don't follow this point. If c is dropped, the parent node that depends on it (directly or several levels above) should be recalculated without c.
If this is significantly harder than doing pair-wise aggregation of the nodes that we will store/share, then we can go with re-aggregating using scipy ward method.

@gschivley Maybe I misunderstood your original suggestion. Indeed, for this to work, incomplete nodes need to be recalculated from their children, rather than taking some fraction of the values pre-computed for those nodes.

@ezwelty Could be that I wasn't clear. I agree that fractional values of the pre-computed nodes wouldn't work - would need to go down to children and re-calculate.

I have a working beta, up here as a gist on GitHub:
https://gist.github.com/ezwelty/6ea30ae1bf9abf8ed3ceb4c860d2b66a
Assumes that clusters.py and test_clusters.py are together in your current python working directory alongside all the data files you have sent me. The only change I have made to the data files are to convert cluster_level to integer in preliminary_base_wind_metadata.csv, so as not to have to support both data types.

@gschivley I am going to need some guidance on what the output should look like (from you or GenX documentation, if such a thing exists). Following the steps of GeneratorClusters.create_new_generators():

startup_fuel(), add_fuel_labels(), and startup_nonfuel_costs() add the following fields and values. I presume this is the desired behavior for renewables.

Start_fuel_MMBTU_per_MW: 0
Fuel: None
Start_cost_per_MW: 0

add_genx_model_tags() adds the following fields and values. Judging from the example settings model_tag_values, these should be tagged as either DISP: 1, RPS: 1, or New_Build: 1. Which should it be? Should the matching of technology to model_tag_values be made case and white space insensitive? What is the meaning of the underscores in some model_tag_values (e.g. UtilityPV_ vs. UtilityPV)?

THERM: 0
DISP: 0
NDISP: 0
STOR: 0
DR: 0
HYDRO: 0
Commit: 0
RPS: 0
CES: 0
New_Build: 0
Hydro_level: 0.0
Cap_size: 1.0

The capacity added by add_resource_max_cap_spur_line() appears to be in MW (Max_Cap_MW). I presume cluster capacity, currently in GW, should be converted to MW? Should user settings for cluster min_capacity be in MW as well?

4a. calculate_transmission_inv_cost() currently requires spur_line_miles, but in the new dataset, you named this spur_miles. Which name should PowerGenome use going forward? Clusters can also have offshore_spur_miles, tx_miles, site_substation_spur_miles, substation_metro_tx_miles, and site_metro_spur_miles. Should each spur and tx mile type have its own user settings for wacc, investment_years, and capex_mw_mile?
4b. add_transmission_inv_cost() requires Inv_cost_per_MWyr. Is this 0 for clusters?

GeneratorClusters.create_all_generators() also requires Heat_rate_MMBTU_per_MWh and Min_power, which I presume should both be 0 for renewables?

Finally, relative to example settings 'generator_columns`, the following fields are still missing. Which are required downstream, and which null (or non-null) value should they have for clusters?

Max_Cap_MW: Can be computed from cluster gw.
Down_time, Eff_down, Eff_up, Existing_Cap_MW,
Existing_Cap_MWh, Externality_cost_MWh,
Fixed_OM_cost_per_MWhyr, Fixed_OM_cost_per_MWyr, HEAT,
Inv_cost_per_MWhyr,
Max_DSM_delay, Max_Duration, Max_Share, Max_Share_percent,
Min_Cap_MW, Min_Duration, Min_Share, Min_Share_percent,
NACC, NACC_Eff, NACC_Peak_to_Base,
Ramp_Dn_percentage, Ramp_Up_percentage,
Ratio_power_to_energy, Reg_Cost, Reg_Dn, Reg_Up,
Resource, Rsv_Cost, Rsv_Dn, Rsv_Up, Self_disch, TRADER,
Up_time, Var_OM_cost_per_MWh, Var_OM_cost_per_MWh_in,
capex, capex_mwh, heat_rate_mmbtu_mwh_iqr,
heat_rate_mmbtu_mwh_std, num_units,
unmodified_existing_cap_mw, voltage_level, zone

p.s. Is all this crazy capitalization required by GenX, or can we use snake_case?

p.s. Is all this crazy capitalization required by GenX, or can we use snake_case?

It's all required by GenX.

startup_fuel(), add_fuel_labels(), and startup_nonfuel_costs() add the following fields and values. I presume this is the desired behavior for renewables.
Start_fuel_MMBTU_per_MW: 0
Fuel: None
Start_cost_per_MW: 0

Yes. I believe the default behavior is to have None or 0 unless a value is specified in the settings.

add_genx_model_tags() adds the following fields and values. Judging from the example settings model_tag_values, these should be tagged as either DISP: 1, RPS: 1, or New_Build: 1. Which should it be?

Current behavior is to let the user specify in settings, but generally wind/solarpv would be 1 for all three of those (DISP is renewables that can be curtailed - e.g. "dispatched" - RPS is eligible for renewable portfolio standards, and New_Build means you can build more of it). The tag names within GenX might change, I think it's best to keep running this through the settings.

Should the matching of technology to model_tag_values be made case and white space insensitive? What is the meaning of the underscores in some model_tag_values (e.g. UtilityPV_ vs. UtilityPV)?

It would be great to make this stuff case and white space insensitive. I used trailing underscores for string-matching with new technologies from ATB in cases where the tech_detail isn't important. As a refresher, ATB gives a technology category (e.g. NaturalGas or UtilityPV) and then tech_detail subcategories. The tech_detail matters for NaturalGas - it's a combustion turbine or combined cycle, which are very different things. But for UtilityPV it's just the name of a city. I can't remember if the trailing underscore is required in all cases, but it can help differentiate new-build resources in ATB from existing resources in EIA data (or user-defined resources). Oh, and in some cases like NaturalGas_ it helps indicate that this is all of tech_detail subcategories of NaturalGas.
So probably not required but maybe I started including it because of a weird edge-case?
Will come back to this later...

CONTINUED...

The capacity added by add_resource_max_cap_spur_line() appears to be in MW (Max_Cap_MW). I presume cluster capacity, currently in GW, should be converted to MW? Should user settings for cluster min_capacity be in MW as well?

Good catch. All of the PG stuff is in MW, it's probably best to switch everything in this pipeline over to MW rather than using GW.

calculate_transmission_inv_cost() currently requires spur_line_miles, but in the new dataset, you named this spur_miles. Which name should PowerGenome use going forward? Clusters can also have offshore_spur_miles, tx_miles, site_substation_spur_miles, substation_metro_tx_miles, and site_metro_spur_miles.

Let's use spur_miles, offshore_spur_miles, and tx_miles. The other distances are used to determine those three.

Should each spur and tx mile type have its own user settings for wacc, investment_years, and capex_mw_mile?

Yes.

add_transmission_inv_cost() requires Inv_cost_per_MWyr. Is this 0 for clusters?

Inv_cost_per_MWyr is the investment cost of just the power plant, and is calculated in nrelatb.atb_new_generators(). It comes in as part of the resource_df input, where interconnection investment costs (the annuity) are added to it.

GeneratorClusters.create_all_generators() also requires Heat_rate_MMBTU_per_MWh and Min_power, which I presume should both be 0 for renewables?

Yes, but these are part of yet another external data file and we don't need to worry about them.

Finally, relative to example settings 'generator_columns`, the following fields are still missing. Which are required downstream, and which null (or non-null) value should they have for clusters?

Max_Cap_MW: Can be computed from cluster gw.
Down_time, Eff_down, Eff_up, Existing_Cap_MW,
Existing_Cap_MWh, Externality_cost_MWh,
Fixed_OM_cost_per_MWhyr, Fixed_OM_cost_per_MWyr, HEAT,
Inv_cost_per_MWhyr,
Max_DSM_delay, Max_Duration, Max_Share, Max_Share_percent,
Min_Cap_MW, Min_Duration, Min_Share, Min_Share_percent,
NACC, NACC_Eff, NACC_Peak_to_Base,
Ramp_Dn_percentage, Ramp_Up_percentage,
Ratio_power_to_energy, Reg_Cost, Reg_Dn, Reg_Up,
Resource, Rsv_Cost, Rsv_Dn, Rsv_Up, Self_disch, TRADER,
Up_time, Var_OM_cost_per_MWh, Var_OM_cost_per_MWh_in,
capex, capex_mwh, heat_rate_mmbtu_mwh_iqr,
heat_rate_mmbtu_mwh_std, num_units,
unmodified_existing_cap_mw, voltage_level, zone

General rule is that anything snake case isn't required by GenX - it's there as extra info for manual inspection. Other than Max_Cap_MW, I think the other columns either don't apply or are included in the external data file I linked above.
Actually, zone is required but is added via a mapping of the region column to integers.

@gschivley

Current behavior is to let the user specify in settings, but generally wind/solarpv would be 1 for all three of those (DISP is renewables that can be curtailed - e.g. "dispatched" - RPS is eligible for renewable portfolio standards, and New_Build means you can build more of it). The tag names within GenX might change, I think it's best to keep running this through the settings.

Looking at the example settings again, it appears they should 1 for DISP, RPS, New_Build, and CES.
Will you modify your upstream clusters to use the term utilitypv instead of solarpv and landbasedwind instead of wind, (and keep using offshorewind), so that we don't need to add yet more terms to the settings? Alternatively, I can make the change on import in PowerGenome.

All of the PG stuff is in MW, it's probably best to switch everything in this pipeline over to MW rather than using GW.

Is that a change you will make upstream (e.g. change gw to mw), or should I make that change on import within PowerGenome?

Let's use spur_miles, offshore_spur_miles, and tx_miles. The other distances are used to determine those three.

Alright, I will mass rename spur_line_... to spur_... in PowerGenome. That is what you are suggesting, yes?
What transmission combinations should be considered? Can you explain "the other distances are used to determine those three"? Because it doesn't seem that simple.

offshore_spur_miles + spur_miles
tx_miles
site_substation_spur_miles + substation_metro_tx_miles
site_metro_spur_miles
... ?

We end up with different ..._capex and ..._inv_mwyr columns based on the different transmission scenarios. Which should be added to Inv_cost_per_MWyr? The cheapest one?

Looking at the example settings again, it appears they should 1 for DISP, RPS, New_Build, and CES.

Yes, but just let this flow through from the settings file.

Will you modify your upstream clusters to use the term utilitypv instead of solarpv and landbasedwind instead of wind, (and keep using offshorewind), so that we don't need to add yet more terms to the settings? Alternatively, I can make the change on import in PowerGenome.

Sure, I can do this.

Is that a change you will make upstream (e.g. change gw to mw), or should I make that change on import within PowerGenome?

I'll make it upstream.

Alright, I will mass rename spur_line_... to spur_... in PowerGenome. That is what you are suggesting, yes?

Yes.

What transmission combinations should be considered? Can you explain "the other distances are used to determine those three"? Because it doesn't seem that simple.

site_substation_spur_miles, substation_metro_tx_miles, and site_metro_spur_miles are all calculated and used to determine costs for either a) interconnection through a substation, or b) direct spur line to a city. I take the minimum of those two values. If direct to metro is cheaper, then site_metro_spur_miles becomes spur_miles and tx_miles is 0. So I'm suggesting that only spur_miles, tx_miles, and offshore_spur_miles are needed by PG. All of these will be pre-calculated in the data. I've been saving site_substation_spur_miles, substation_metro_tx_miles, and site_metro_spur_miles but they aren't strictly necessary.

add_transmission_inv_cost() requires Inv_cost_per_MWyr. Is this 0 for clusters?

Inv_cost_per_MWyr is the investment cost of just the power plant, and is calculated in nrelatb.atb_new_generators(). It comes in as part of the resource_df input, where interconnection investment costs (the annuity) are added to it.

I understand where it comes from for the current "new generators". My question is how it should be handled for these renewables clusters. The behavior of atb_new_generators() is currently controlled by the following setting (, <tech_detail>, <cost_case>, ):
atb_new_gen:
  - [NaturalGas, CCCCSAvgCF, Mid, 500]
  - [NaturalGas, CCAvgCF, Mid, 500]
  - [NaturalGas, CTAvgCF, Mid, 100]
  - [LandbasedWind, LTRG1, Mid, 1]
  - [OffShoreWind, OTRG3, Mid, 1]
  - [UtilityPV, LosAngeles, Mid, 1]
  - [Battery, "*", Mid, 1]

If there is one scenario for each of utilitypv, offshorewind, and landbasedwind, I can apply the current calculation to compute the investment cost of the power plant for each cluster. Can I count on this being true, and throw an error otherwise? If there is more than one scenario, do I generate multiple versions of each cluster, or throw an error?

If there is one scenario for each of utilitypv, offshorewind, and landbasedwind, I can apply the current calculation to compute the investment cost of the power plant for each cluster. Can I count on this being true, and throw an error otherwise? If there is more than one scenario, do I generate multiple versions of each cluster, or throw an error?

By "scenario", do you mean within a specific year/scenario combination that are defined in the file that scenario_definitions_fn points to? The default for PG is currently that all new generators are available in all years. So the number of rows doesn't change across years or scenarios, although some values (like investment cost) will change across years.
I suppose the existing framework is flexible enough that a user could use settings_management to change the resources that are available in a given year/scenario. But I don't think that's what you're talking about.
Do you mean will the costs be Mid or Low across all regions?

@gschivley

If there is one scenario for each of utilitypv, offshorewind, and landbasedwind, I can apply the current calculation to compute the investment cost of the power plant for each cluster. Can I count on this being true, and throw an error otherwise? If there is more than one scenario, do I generate multiple versions of each cluster, or throw an error?

By "scenario", do you mean within a specific year/scenario combination that are defined in the file that scenario_definitions_fn points to? The default for PG is currently that all new generators are available in all years. So the number of rows doesn't change across years or scenarios, although some values (like investment cost) will change across years.

By "scenario", I meant each (technology, tech_detail, cost_case, size) in settings.atb_new_gen.

Yes, there is one (technology, tech_detail, cost_case, size) for each new resource. Each of these is available in every model region unless it is indicated in settings.new_gen_not_available. The capex for each resource is modified by region in powergenome.nrelatb.regional_capex_multiplier(), but otherwise the resources are identical across regions within a year/scenario.
One change we need to make (I'm sure you're aware of this) is to use the actual number of clusters per region instead of the existing settings.new_wind_solar_regional_bins.

@ezwelty I got a request to make it so we can tie clusters that are selected by PG back to individual sites (CPAs), so that someone can make maps of the CPAs that were (likely) built in each planning period. This shouldn't be a large change to your workflow but it'll mean saving some additional data files with the geometry, LCOE, and associated cluster of each site.

One change we need to make (I'm sure you're aware of this) is to use the actual number of clusters per region instead of the existing settings.new_wind_solar_regional_bins.

My assumption has been that the new renewables clusters would get tacked on to the unchanged current output of GeneratorClusters. However, it sounds like you are saying that I should instead be inserting them where add_extra_wind_solar_rows() currently stands in the GeneratorClusters.create_new_generators() workflow? (Or at least dropping settings.new_wind_solar_regional_bins in the current output and appending the new clusters?)

Yes, there is one (technology, tech_detail, cost_case, size) for each new resource. Each of these is available in every model region unless it is indicated in settings.new_gen_not_available. The capex for each resource is modified by region in powergenome.nrelatb.regional_capex_multiplier(), but otherwise the resources are identical across regions within a year/scenario.

I am currently testing with the following additional settings for renewables clusters. I guess settings.new_gen_not_available could take precedence, forcing clusters to be dropped if they match on region and technology.
{
    "renewables_clusters_path": "...",
    "renewables_clusters": [
        {
            "region": "CA_N",
            "ipm_regions": ["NENGREST", "PJM_Dom"],
            "technology": "offshorewind",
            "turbine_type": "fixed",
            "pref_site": 1,
            "min_capacity": None,
            "max_clusters": 3,
        },
        {
            "region": "CA_N",
            "ipm_regions": ["NENGREST", "PJM_Dom"],
            "technology": "offshorewind",
            "turbine_type": "fixed",
            "pref_site": 0,
            "min_capacity": 10,
            "max_clusters": None,
        },
    ]
}

My assumption has been that the new renewables clusters would get tacked on to the unchanged current output of GeneratorClusters. However, it sounds like you are saying that I should instead be inserting them where add_extra_wind_solar_rows() currently stands in the GeneratorClusters.create_new_generators() workflow? (Or at least dropping settings.new_wind_solar_regional_bins in the current output and appending the new clusters?)

We need to let a user specify the tech_detail and cost_case for each renewable type. I think it makes sense to keep this in settings.atb_new_gen with the other technologies. We need a place to specify how many clusters should be in each model region, which used to be settings.new_wind_solar_regional_bins. With the new system we don't need that anymore. There are better ways to insert the extra rows than add_extra_wind_solar_rows(), but it exists and works... ¯_(ツ)_/¯

I am currently testing with the following additional settings for renewables clusters (ipm_regions is a placeholder that will later be populated automatically internally). I guess settings.new_gen_not_available could take precedence, forcing clusters to be dropped if they match on region and technology.

Looks good. I don't think that settings.new_gen_not_available needs to be as important if we remove* rows in regions that aren't specified in your new settings section.

A row is added in each region for every technology by default.

@gschivley I have inserted the renewables clusters into the GeneratorClusters.create_all_generators() workflow (see https://github.com/gschivley/PowerGenome/pull/42/commits/fae5a6332a1c5036518d5d056ad5089f75f1efa5).
A few thoughts/questions:

If settings.renewables_clusters does not list anything for a certain region and technology matching an entry in settings.atb_new_gen, the placeholder row created for that entry in settings.atb_new_gen remains in place. Is this reasonable behavior?
If the user wants cluster interconnect_annuity to override custom per-mile costs, should it be added in place of the sum of {spur|offshore_spur|tx}_inv_mwyr columns to Inv_cost_per_MWyr?
You said that NREL has offshore spur costs that go down over time. I will probably need more details from you when the time comes to update calculate_transmission_inv_cost() accordingly.
This single linear workflow is probably not sustainable long term. I envision PowerGenome will grow more easily if each new type of resource is built independently from shared modular computational steps, combined downstream, and a single "format_for_genx()" function adds, formats, fills, and renames columns as needed for GenX. The sporadic rounding, renaming, type casting, and na-filling adds a lot of brittleness and noise to the code.

If settings.renewables_clusters does not list anything for a certain region and technology matching an entry in settings.atb_new_gen, the placeholder row created for that entry in settings.atb_new_gen remains in place. Is this reasonable behavior?

No, don't leave a row if a region doesn't have the technology option in settings.renewables_clusters. Otherwise we have single rows with no information on the max capacity or interconnection costs and it'll screw up models.

If the user wants cluster interconnect_annuity to override custom per-mile costs, should it be added in place of the sum of {spur|offshore_spur|tx}_inv_mwyr columns to Inv_cost_per_MWyr?

Yes. I'm assuming that interconnection_annuity will be used by default. Somehow we need to let users specify spur line distance for non-renewable technologies, and that's a place they can also specify renewable {spur|offshore_spur|tx} distances. Happy to talk about where this should be moved since it's currently in a csv file.

You said that NREL has offshore spur costs that go down over time. I will probably need more details from you when the time comes to update calculate_transmission_inv_cost() accordingly.

I can add this to the custom PUDL database since it's part of ATB. Assume there will be a table named something like offshore_spur_cost_nrelatb. It will have the cost of 30km offshore spur line (what NREL says they assume). (EDIT: I just updated the custom pudl database with this table)
And crap... I have to subtract that 30km offshore spur cost from capex in the PUDL database. Will be doing that tonight.

This single linear workflow is probably not sustainable long term. I envision PowerGenome will grow more easily if each new type of resource is built independently from shared modular computational steps, combined downstream, and a single "format_for_genx()" function adds, formats, fills, and renames columns as needed for GenX. The sporadic rounding, renaming, type casting, and na-filling adds a lot of brittleness and noise to the code.

Yeah, I agree.

If settings.renewables_clusters does not list anything for a certain region and technology matching an entry in settings.atb_new_gen, the placeholder row created for that entry in settings.atb_new_gen remains in place. Is this reasonable behavior?

No, don't leave a row if a region doesn't have the technology option in settings.renewables_clusters. Otherwise we have single rows with no information on the max capacity or interconnection costs and it'll screw up models.

Done.

If the user wants cluster interconnect_annuity to override custom per-mile costs, should it be added in place of the sum of {spur|offshore_spur|tx}_inv_mwyr columns to Inv_cost_per_MWyr?

Yes. I'm assuming that interconnection_annuity will be used by default. Somehow we need to let users specify spur line distance for non-renewable technologies, and that's a place they can also specify renewable {spur|offshore_spur|tx} distances. Happy to talk about where this should be moved since it's currently in a csv file.

Done. Currently, spur_miles, max_capacity (renamed Max_Cap_MW) and cluster (?) are still added from settings.capacity_limit_spur_fn for non-renewables in add_resource_max_cap_spur(). This could be revised to include offshore_spur_miles and tx_miles. Alternatively, it could be moved to a redesigned settings.atb_new_gen, since that is where the non-renewable NREL ATB technologies are requested by the user.
The field is called interconnect_annuity in the current clusters, but maybe this field should be renamed transmission_inv_mwyr for consistency with existing fields.

@gschivley Incorporated the hourly generation profiles in https://github.com/gschivley/PowerGenome/pull/42/commits/5f03813b28eb3a2b64dfcefbadaabd1a8e5cf410.
I ended up passing through cluster profiles in a new variability column of the resource table and happy with that solution. make_generator_variability() reads these out and fills in missing values with any matching profile from file. Something I didn't mention in the commit message is that any profiles of length 8784 are shortened to 8760 by dropping February 29.
Let me know when the offshore_spur data is ready to be slotted in as a default for settings.transmission_investment_cost.offshore_spur.capex_mw_mile.

Something I didn't mention in the commit message is that any profiles of length 8784 are shortened to 8760 by dropping February 29.

Nice! We need to make sure this happens before time domain reduction, but I don't think that's an issue.

Let me know when the offshore_spur data is ready to be slotted in as a default for settings.transmission_investment_cost.offshore_spur.capex_mw_mile.

The table is in the live version of the database. I also removed all of the hourly CEMS data so db size is ~300 MB rather than over 1 GB.

Something I didn't mention in the commit message is that any profiles of length 8784 are shortened to 8760 by dropping February 29.

Nice! We need to make sure this happens before time domain reduction, but I don't think that's an issue.

That's already the case, since it happens in make_generator_variability().

Let me know when the offshore_spur data is ready to be slotted in as a default for settings.transmission_investment_cost.offshore_spur.capex_mw_mile.

The table is in the live version of the database. I also removed all of the hourly CEMS data so db size is ~300 MB rather than over 1 GB.

I thought you still needed to subtract something and multiply by 1 mile / 30 km in https://github.com/gschivley/PowerGenome/tree/existing_renewables_voronoi.

I thought you still needed to subtract something and multiply by 1 mile / 30 km in https://github.com/gschivley/PowerGenome/tree/existing_renewables_voronoi.

I added a capex_mw_mile column to the dataframe (https://github.com/gschivley/PowerGenome/commit/177a0a4ad322644337fbd7f9e44568c180c2406b). Now I can easily subtract the entire value from offshorewind capex and you can use the per MW-mile value.

@gschivley I just merged in existing_renewables_voronoi(https://github.com/gschivley/PowerGenome/commit/177a0a4ad322644337fbd7f9e44568c180c2406b), which was a little tricky. I think I smoothed out the kinks, although the logic around interconnect_annuity may require additional review.
Are you aware that the example is currently failing? The culprit appears to be that capex for all OffShoreWind in atb_costs is NaN. I believe that issue is upstream of any changes I made.

@gschivley Alright, I think this is now all working as expected. You should probably run the example and inspect the output for any red flags. I had to make a few patches to code merged in from existing_renewables_voronoi to get the whole thing to run (https://github.com/gschivley/PowerGenome/pull/42/commits/53378b4590c3e72bedaaff1802b982125b39afe6, https://github.com/gschivley/PowerGenome/pull/42/commits/464da3237f46bf7066214f9bad4d6c84d44f8264, https://github.com/gschivley/PowerGenome/pull/42/commits/bc39b906571a7763c76476101b7613754164b665: filling NA in float columns before conversion to integer).

I'll see if I have time to run tonight. I've also written a function that calculates hourly profiles for hydro by region (starts with monthly values, reindexes them to hourly, then does a rolling mean to smooth it out). Once we combine this hydro with the new-build renewable clusters and existing renewables we'll be able to get rid of the whole resource_variability_fn parameter.

@gschivley Here is a proposal for generalizing the clusters data format implemented in this pull request to a more general data format for representing a group of power-generating resources. I wrote it out in detail in the hopes that it can serve as a starting point for fine-tuning. In particular, take a look at the scenarios at the end, and tell me if the list is complete and what additional or different functionality should be supported.
Group metadata (*_group.json)

technology (str) – Technology type. Although not strictly required, it is needed to interface with other data sources like the NREL Annual Technology Baseline (ATB). The following values are currently supported:

utilitypv
landbasedwind
offshorewind – for which the attribute turbine_type is also required (either fixed or floating).

existing (Optional[bool]) – Whether the resources are new (false) or existing (true). Defaults to false. Although not strictly required, this distinction allows PowerGenome to handle each differently.
clustered (Optional[Union[str, bool]]) – Whether the resource group represents a precomputed hierarchical clustering tree (true) or the name of the resource attribute (in the metadata file) by which to differentiate between multiple hierarchical trees (str). Defaults to false.
... (Optional[Any]) – Additional group attributes for record keeping and/or group selection.

Resource metadata (*.csv)
Each resource is described in a row of a csv file with the following attributes.

ipm_region (str) – IPM region to which the resource delivers power.
mw (float) – Maximum resource capacity in MW.
id (Optional[int]) – Resource identifier, unique within the group. Defaults to the row index (0, ..., n - 1), where n is the number of resources in the group.
lcoe (Optional[float]) - Levelized cost of energy, used to guide the selection (from lowest to highest) and clustering (by nearest) of resources. If missing, selection and clustering is by row index.

Resources representing hierarchical trees require additional attributes.

parent_id (Optional[int]) – id of the resource formed by clustering this resource with the one other resource with the same parent_id. Only resources with cluster_level: 1 have no parent_id. Defaults to null.
cluster_level (Optional[int]) – Cluster level where the resource first appears, from m (the number of resources at the base of the tree), to 1. Defaults to 1.
[group.clustered] (Optional[Any]) – Each unique value of this grouping attribute represents a precomputed hierarchical tree. When clustering resources, every tree is traversed to its crown before the singleton resources from the trees are clustered together.

The calculation of clustered resource attributes is hardcoded by attribute name:

Weighted means:

lcoe
interconnect_annuity
tx_miles
spur_miles
offshore_spur_miles
site_substation_spur_miles
substation_metro_tx_miles
site_metro_spur_miles

Weight for the weighted means:

mw

Sums:

mw
area

Resource profiles (*.parquet or *.csv)
Resource capacity variability is represented as a list of normalized factors (from 0 to 1) for every hour of the year (either 8760 or 8784 for a leap year).
Seemingly due to bugs in pyarrow, filtering of parquet files is unreliable. Therefore, storing each profile in a column is recommended for speed, memory efficiency, and compatibility with both parquet and csv:

resource.id[0]
...
resource.id[n - 1]

capacity_factor(resource.id=0, hour=0)
...
capacity_factor(resource.id=n - 1, hour=0)

...
...
...

capacity_factor(resource.id=0, hour=8760 - 1)
...
capacity_factor(resource.id=n - 1, hour=8760 - 1)

where resource.id is the value of id in the resource metadata (or the resource row index if id is missing).
Scenarios

Multiple precomputed hierarchical trees

Group metadata

clustered: 'metro_id'

Resource metadata

ipm_region, mw, id, metro_id
parent_id: { id }
cluster_level: { 1, ... , m }

Process

Select resources by

matching resource.ipm_region, and
maximum resource.cluster_level for each resource[group.clustered].

Order by ascending lcoe (if present).
Select in order until min_capacity is reached.
If max_clusters is not met,

cluster following the precomputed tree for each resource[group.clustered], then, if needed,
cluster by nearest lcoe (if present) or by row index.

Single precomputed hierarchical tree

Group metadata

clustered: true

Resource metadata

ipm_region, mw, id
parent_id: { id }
cluster_level: { 1, ... , m }

Process

Select resources by

matching resource.ipm_region, and
maximum resource.cluster_level.

Order by ascending lcoe (if present).
Select in order until min_capacity is reached.
If max_clusters is not met,

cluster following the precomputed tree, then, if needed,
cluster by nearest lcoe (if present) or by row index.

Single or multiple resources for each IPM region

Group metadata

clustered: false (default)

Resource metadata

ipm_region, mw

Process

Select resources by

matching resource.ipm_region.

Order by ascending lcoe (if present).
Select in order until min_capacity is reached.
If max_clusters is not met,

cluster by nearest lcoe (if present) or by row index.

A note so that I don't forget - within ClusterBuilder we need to specify the extra resource characteristics like turbine_type and pref_site value to help associate the ids back to a specific resource. Just OffShoreWind isn't enough since id isn't unique across the different permutations.

A note so that I don't forget - within ClusterBuilder we need to specify the extra resource characteristics like turbine_type and pref_site value to help associate the ids back to a specific resource. Just OffShoreWind isn't enough since id isn't unique across the different permutations.

ClusterBuilder is agnostic to resource group attributes – they don't need to be hard coded. Either you provide enough attributes/values to uniquely identify a resource group, or you don't and you receive an error. The **kwargs of ClusterBuilder.build_clusters are by definition the attributes that define the resource group.

A note so that I don't forget - within ClusterBuilder we need to specify the extra resource characteristics like turbine_type and pref_site value to help associate the ids back to a specific resource. Just OffShoreWind isn't enough since id isn't unique across the different permutations.

ClusterBuilder is agnostic to resource group attributes – they don't need to be hard coded. Either you provide enough attributes/values to uniquely identify a resource group, or you don't and you receive an error. The **kwargs of ClusterBuilder.build_clusters are by definition the attributes that define the resource group.
I should have been more explicit. The current output from ClusterBuilder().get_cluster_metadata() doesn't retain any information about the attributes/values other than the resource name. Since each permutation of OffShoreWind (turbine_type/pref_site) will have cluster id values that begin with 1 and increase sequentially, there could be 4 different sets of cpa_ids for each unique OffShoreWind cluster id. When a user tries to trace back to the CPAs within an OffShoreWind cluster they will run into issues.

@gschivley Got it. If you want the metadata to uniquely identify the resource (group and id in the group), we can append those **kwargs to the resource metadata table (as we already do for the most common of them: technology, aka Resource).

@gschivley I've implemented the scheme previously described. The only changes are that group.clustered is always a string (i.e. the name of the column which designates each hierarchical tree, essentially dropping support for the single tree scenario which you said would not be needed) and when lcoe is not available, resources are selected (for min_capacity) by descending mw order, and clustered by nearest mw. Let me know if this latter design change is acceptable.
In the process, I generalized renewables_clusters.py. The Table class provides a cached interface to csv and parquet files. The ResourceGroup class implements the schema I outlined. Clustering and merging of resources is performed by merge_rows() (generalized merging of dataframe rows), cluster_rows() (generalized merging of dataframe rows by hierarchical clustering), and cluster_row_trees()(generalized merging of dataframe rows following precomputed hierarchical trees). I will use these former two to rewrite cluster creation incluster_areas.py`.

@ezwelty I'm getting an error when trying to create new resources using the test_settings.yml file and these input files.
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
<ipython-input-37-79bde7d94c95> in <module>
----> 1 new_gen = gc.create_new_generators()

~/Documents/CATF/PowerGenome/powergenome/generators.py in create_new_generators(self)
   2338
   2339         self.new_generators = atb_new_generators(
-> 2340             self.atb_costs, self.atb_hr, self.settings
   2341         )
   2342

~/Documents/CATF/PowerGenome/powergenome/nrelatb.py in atb_new_generators(atb_costs, atb_hr, settings)
    740             regional_cost_multipliers,
    741         )
--> 742         _df = add_renewables_clusters(_df, region, settings)
    743
    744         if region in (settings.get("new_gen_not_available") or {}):

~/Documents/CATF/PowerGenome/powergenome/nrelatb.py in add_renewables_clusters(df, region, settings)
    834         technology = technologies[0]
    835         builder = ClusterBuilder.from_path(SETTINGS["RENEWABLES_CLUSTERS"])
--> 836         builder.build_clusters(**scenario, ipm_regions=ipm_regions)
    837         profiles = builder.get_cluster_profiles()
    838         clusters = (

~/Documents/CATF/PowerGenome/powergenome/renewables_clusters.py in build_clusters(self, region, ipm_regions, min_capacity, max_clusters, max_lcoe, cap_multiplier, **kwargs)
    721                 max_clusters=max_clusters,
    722                 max_lcoe=max_lcoe,
--> 723                 cap_multiplier=cap_multiplier,
    724             ),
    725         }

~/Documents/CATF/PowerGenome/powergenome/renewables_clusters.py in get_clusters(self, ipm_regions, min_capacity, max_clusters, max_lcoe, cap_multiplier)
    528                 df[mask | ~base], by=by, tree=tree, max_rows=max_clusters, **merge
    529             )
--> 530         return cluster_rows(df[mask], by=df[[by]], max_rows=max_clusters, **merge)
    531
    532     def get_cluster_profiles(self, ids: Iterable[Iterable]) -> np.ndarray:

~/Documents/CATF/PowerGenome/powergenome/renewables_clusters.py in cluster_rows(df, by, max_rows, **kwargs)
    951     mask = [True] * nrows
    952     for i, link in enumerate(Z[:drows, 0:2].astype(int)):
--> 953         mask[link[0]] = False
    954         mask[link[1]] = False
    955         df.loc[nrows + i] = pd.Series(merge_rows(df.loc[link], **kwargs))

IndexError: list assignment index out of range

Here's the pdb output - shows some nan rows which seems odd.
> /Users/greg/Documents/CATF/PowerGenome/powergenome/renewables_clusters.py(953)cluster_rows()
    951     mask = [True] * nrows
    952     for i, link in enumerate(Z[:drows, 0:2].astype(int)):
--> 953         mask[link[0]] = False
    954         mask[link[1]] = False
    955         df.loc[nrows + i] = pd.Series(merge_rows(df.loc[link], **kwargs))

ipdb>  link
array([117, 118])
ipdb>  nrows
105
ipdb>  df
    ipm_region metro_id  cluster_level  cluster      lcoe       area  \
0     WEC_CALN    23420           20.0     11.0   84.1046   127.7500
1     WEC_CALN    41860           20.0      1.0   90.7374   179.5405
2     WEC_CALN    41940           20.0     16.0   99.9318   548.2500
3     WEC_CALN    23420           19.0     11.0  101.7764  1682.0309
4     WEC_CALN    23420           20.0     12.0  103.2289  1554.2809
..         ...      ...            ...      ...       ...        ...
202        NaN      NaN            NaN      NaN       NaN        NaN
203        NaN      NaN            NaN      NaN       NaN        NaN
204        NaN      NaN            NaN      NaN       NaN        NaN
205        NaN      NaN            NaN      NaN       NaN        NaN
206        NaN      NaN            NaN      NaN       NaN        NaN

            mw     rmse  relative_rmse  site_substation_spur_miles  \
0     344.9250   5.4178         0.0644                     30.4581
1     484.7594  28.2982         0.3119                     59.2272
2    1480.2750   6.6394         0.0664                      6.7647
3    4541.4835  34.8918         0.3428                     33.6841
4    4196.5585   9.5365         0.0924                     33.9492
..         ...      ...            ...                         ...
202        NaN      NaN            NaN                         NaN
203        NaN      NaN            NaN                         NaN
204        NaN      NaN            NaN                         NaN
205        NaN      NaN            NaN                         NaN
206        NaN      NaN            NaN                         NaN

     substation_metro_tx_miles  site_metro_spur_miles  spur_miles  tx_miles  \
0                      78.1993                80.3279     30.4581   78.1993
1                     118.9015               146.3860     59.2272  118.9015
2                      26.4733                21.9562      7.5499   22.4357
3                      76.5884               103.1159     33.6841   76.5884
4                      76.4560               104.9889     33.9492   76.4560
..                         ...                    ...         ...       ...
202                        NaN                    NaN         NaN       NaN
203                        NaN                    NaN         NaN       NaN
204                        NaN                    NaN         NaN       NaN
205                        NaN                    NaN         NaN       NaN
206                        NaN                    NaN         NaN       NaN

     interconnect_annuity  m_popden meets_criteria  parent_id
0              38557.7242    0.4070          False     3371.0
1              67283.7413    3.6123          False     3459.0
2              10265.6647    0.2160          False     3493.0
3              40346.2417    0.1777          False     3382.0
4              40493.2441    0.1589          False     3371.0
..                    ...       ...            ...        ...
202                   NaN       NaN            NaN        NaN
203                   NaN       NaN            NaN        NaN
204                   NaN       NaN            NaN        NaN
205                   NaN       NaN            NaN        NaN
206                   NaN       NaN            NaN        NaN

[207 rows x 18 columns]

@gschivley Yes, sorry, the specs for the input files (resource groups) have changed (are changing) slightly and I have not yet updated cluster_areas.py accordingly. If you need to have in run on the current state of this branch in a hurry:

Add tree: "metro_id" to the group metadata.
Rename cluster_level to level in the resource metadata.

Got it, thanks! So does this workflow require a whole set of group/metadata/profiles files to create the new resources in nrelatb.addrenewables_clusters? As I'm working on some examples and documentation it'll help to know if a user can just create the new resource dataframe without hourly profiles, or if the two are always connected.

Take a look at the docstring for ResourceGroup, which lays out the format spec. Currently profiles are required. Are you requesting that they be made optional?

Are you requesting that they be made optional?

Now that I think about it, no. We need the interconnection costs to calculate investment for renewable resources. And those come with the metadata files. Maybe I'll write something that lets it skip the profile creation if the parquet files aren't found, but don't worry about it.

I just noticed that make_generator_variability is still using the external file from settings resource_variability_fn. If we can, I'd like to get rid of resource_variability_fn, because the only thing still coming from it is demand response profiles. The function external_data.make_demand_response_profiles already loads these from the file demand_response_fn - although they are in MW units rather than 0-1.
Easy enough to pull in some code like this:
from powergenome.external_data import make_demand_response_profiles
from pathlib import Path

# Assumes that we have already tested to make sure demand_response_fn  and
# demand_response_resources exist in settings
dr_path = Path(settings["input_folder"]) / settings["demand_response_fn"]

# Use the demand_response_resources parameter and model year to load all of the different
# demand response resource types
resources = gc.settings["demand_response_resources"][gc.settings["model_year"]].keys()
for resource in resources:
    dr_profiles = make_demand_response_profiles(dr_path, resource, gc.settings)
    dr_profiles = dr_profiles / dr_profiles.max()
    # Now do something to assign these to the `variability` column

    # Just me making sure that I've created normalized profiles
    assert dr_profiles.max().max() == 1

          CA_N      CA_S
0     0.089249  0.089300
1     0.083333  0.083399
2     0.032116  0.032127
3     0.024341  0.024390
4     0.015382  0.015473
...        ...       ...
8755  0.621535  0.621689
8756  0.604462  0.604511
8757  0.511832  0.511802
8758  0.089249  0.089300
8759  0.089249  0.089300

[8760 rows x 2 columns]

@gschivley make_generator_variability only uses resource_variability_fn to pull in variable capacity profiles from file, and nothing else. I kept that feature when I rewrote the function. If we no longer want to pass profiles through resource_variability_fn, I am happy to cut that from the function, which will be greatly simplified as a result.

@gschivley Just pushed my changes. API changes to watch out for:

Renamed renewables_clusters.py to resource_clusters.py
Renamed RENEWABLES_CLUSTERS to RESOURCE_GROUPS in .env
Replaced ClusterBuilder.from_path(path) with ClusterBuilder.from_json(paths)
Removed resource_variability_fn and updated `make_generator_variability accordingly
Removed region= from ClusterBuilder.build_clusters
Replaced ClusterBuilder.build_clusters (cached) with ClusterBuilder.get_clusters (returns result)

@gschivley make_generator_variability only uses resource_variability_fn to pull in variable capacity profiles from file, and nothing else. I kept that feature when I rewrote the function. If we no longer want to pass profiles through resource_variability_fn, I am happy to cut that from the function, which will be greatly simplified as a result.

Let's cut it. Any resource variability should come through the new resource_clusters.py.

@ezwelty First pass of the existing profiles looks good (I pushed a bug-fix related to non-sequential index values). Will put together all of the resources and try out later!

@ezwelty Take a look at this attempt to add the demand response hourly profiles when creating a new resource dataframe. It worked for the test settings but I'd like a second set of eyes.

@gschivley The logic seems sound to me. See my code comment.
Apparently can't comment on existing code, apparently, so writing those down here:
if isinstance(parameters["parameter_values"], dict):

What else would it be but a dict? Do you mean "parameters_values" in parameters or parameters.get("parameter_values")? As written, this will raise an error if parameter_values is not in parameters.
if self.settings.get("demand_response_resources"):
    ...
else:
    dr_rows = pd.DataFrame()
return dr_rows

A nitpick, but I would personally do:
if not self.settings.get("demand_response_resources")`
    return pd.DataFrame()
...

Since this avoids the large indented block and the else.
p.s. Honestly, this feel too complicated. I think the code would be simpler if you first formatted all the input data into the format [ technology | year | scenario | region | **parameters | profile ] and then filtered the rows based on the settings.
You can start from the file format you have now, or with something more like ResourceGroup (metadata: [ id | technology | year | scenario | region | profile | **parameters ], and profiles: [ id[0] | ... ]), in which case you could even use ResourceGroup.get_clusters() with all arguments None to get the profiles loaded in.

Thanks, I incorporated most of your feedback from above.

p.s. Honestly, this feel too complicated. I think the code would be simpler if you first formatted all the input data into the format [ technology | year | scenario | region | **parameters | profile ] and then filtered the rows based on the settings.

You can start from the file format you have now, or with something more like ResourceGroup (metadata: [ id | technology | year | scenario | region | profile | **parameters ], and profiles: [ id[0] | ... ]), in which case you could even use ResourceGroup.get_clusters() with all arguments None to get the profiles loaded in.

We can talk about this as a future improvement, but I'm not sure it's worth messing with right now. Eventually we do want to internalize these demand response generation profiles the same way we did renewables, but it's outside current scope.

@ezwelty I've run the whole pipeline and am satisfied that everything works (or at least nothing is obviously broken). Is there anything else you want to do before we merge?


Add Builder::whitelist_type; deprecate Builder::whitelisted_type

The CLI flag is --whitelist-type, so we should be consistent and remove the "ed" from the builder method.

[ ] Rename Builder::whitelisted_type to Builder::whitelist_type
[ ] Add Builder::whitelisted_type that delegates to whitelist_type
[ ] Mark whitelisted_type as #[deprecated = "use whitelist_type instead"]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.
If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. :smile:

Going to grab this one, then maybe start looking at some of the slightly more difficult ones.
@highfive: assign me

Hey @treiff! Thanks for your interest in working on this issue. It's now assigned to you!


Setting Viewport Help Wanted

Hi. I am trying to load inline SVG file and want to use the SVG properties in the file:
viewBox="0 0 1920 1176.643" preserveAspectRatio="xMinYMid slice"
I know that I need to do this using a 'g' tag like the following link instructs you to do:
https://github.com/ariutta/svg-pan-zoom#using-a-custom-viewport
So I have this started in my inline SVG:
<g class="svg-pan-zoom_viewport" ....... ">
And I have in my SvgPanZoom constructor:
viewportSelector: '.svg-pan-zoom_viewport'
What do I need to add after the class declaration to make this happen? Many thanks in advance!

Hi @FrazerDH ,
Please provide a reproducible example. Without it it's hard to understand what's the issue.
Also please provide some more information on what exactly you're trying to achieve and what exactly is not working. You mentioned:

Hi. I am trying to load inline SVG file and want to use the SVG properties in the file

Does it mean:

Loading an inline SVG doesn't work? If so then most probably this library has nothing to do with it.
You want your inline SVG to have viewBox and preserveAspectRatio? The library removes the viewBox property. See this and #147 .
Nothing works? Then probably the library is not initialised correctly.

@bumbu - Thanks for getting back. I want to initialize my SVG so that it fill my entire div. To do this a zoom number would have to be applied. In other words - I am fitting a square SVG inside a rectangular div.
Right now the svg-pan-zoom library automatically decreases the width of my svg to match the height of the div.
Can I change this behaviour?

mark


[Jammy] Test, Verify, and Update cg-hardening/FISMA release

In order to keep our system dependencies up to date and be ready for security patches, we want to verify that our hardening/FISMA bosh release is compatible with the Ubuntu Jammy stemcell
Acceptance Criteria

[ ] Using a test manifest/deployment, we demonstrate the hardening bosh release compiles on Ubuntu Jammy
[ ] Using a test deployment, verify that that the hardening scripts run successfully in a VM running on Ubuntu Jammy
[ ] Update runtime config to include hardening bosh release as part of for Ubuntu Jammy VMs

[ ] If necessary, updated/change the manifest properties for release

Security considerations
Verifying our compatibility with Ubuntu Jammy will help us stay up-to-date on security patches
Resources

hardening/FISMA bosh release: https://github.com/cloud-gov/cg-harden-boshrelease
Example manifest for deploying a single release on Ubuntu Jammy (replace clamAV with release of choice): https://github.com/cloud-gov/cg-deploy-bosh/blob/89a0f7908462ee961cdf89d197d2027b1b208038/empty-vm.yml
Example of adding ClamAV to runtime config for Jammy: https://github.com/cloud-gov/cg-deploy-bosh/pull/500

The current FISMA release compiled/deployed on Jammy and I saw this output in the post-deploy-stdout.log for the harden job in the deployed VM:
---> Finished hardening process
---> Updating AIDE db

So I think testing has confirmed that the FISMA release is working


Wallet connect using Lilico redirects to chrome extension portal

Problem
When trying to connect using Lilico, although the chrome extension is installed it is not defected and browser navigates to chrome extension to install it.
Steps to Reproduce
List of services provided by fcl.discovery.authn.subscribe()
Lilico gets the following:

where is_installed: false and requeres_install: true
Lilico is installed:

Acceptance Criteria
Should be possible to connect and pop up for the plug-in should open
Context

Addressed here: https://github.com/onflow/fcl-discovery/issues/117


Non-shaded protobuf-java causing class mismatch issues

The Pulsar client lib does not shade com.google.protobuf:protobuf-java and it is causing user's problems as described in original post by @nandorholozsnyak in https://github.com/spring-projects/spring-pulsar/issues/114#issuecomment-2399976242_
Workaround 1
Upgrade to Pulsar client 3.3.2 as he protobuf-java has been updated to 3.25.5.
If you plan on updating to the upcoming spring-pulsar 1.2.0-RC1 release (Spring Boot 3.4.0-RC1 / week of Oct 18) you will get this update automatically.
If you don't plan on updating Spring Pulsar then, you can  override the version of Pulsar as described here.
This will bring the protobuf-java up to a recent version, solving the original author's issue, but will not fix future version mismatches.
Workaround 2
You can exclude protobuf-java from the current pulsar-client-all and provide the more recent dependency (example here.
Something like:
api ("org.apache.pulsar:pulsar-client-all") {
	exclude group: 'com.google.protobuf:protobuf-java'
}
api "com.google.protobuf:protobuf-java:3.25.5"

Improvement 1
We already exclude some other unwanted dependencies from the Pulsar lib (here.
We should likely exclude protobuf-java as well as we do include it explicitly already as optional dependency (here.
We will try to get the above improvement into the upcoming 1.2.0-RC1 release as well.
Improvement 2
We should also see about shading the protobuf-java lib in the Pulsar client libs to avoid this downstream required massaging.

Hello @onobc
Thank you for the issue here too, just a quick question for clarity: The Pulsar client lib does not shade com.google.protobuf:protobuf-java and it is causing user's problems as described in original post
Indeed the pulsar-client lib is not shading it, but is it the real issue? If it would it may break others as well isnt it?

Hi @nandorholozsnyak

pulsar-client lib is not shading it, but is it the real issue? If it would it may break others as well isnt it?
Indeed the pulsar-client lib is not shading it, but is it the real issue?

If one were to pull spring-pulsar out of the equation and consume the pulsar-client lib directly, they would transitively consume the older version of protobuf-java which could cause problems.

Hi @onobc
Thank you so much for the info.

Looks like this may get fixed in Pulsar proper as well: https://github.com/apache/pulsar/pull/23468

Hehe, it is happening :D What a coincidence.


Stryker init creates a temp folder

Running stryker init creates a temp folder that is not removed:

Do you want to not to create this folder or do you want to delete this folder after test run finish?

Both are fine by me :) The interesting thing to figure out is why the folder is created in the first place.

Ah so it is not planned. Ok, ty.

Can you suggest anything so I can debug some npm package? For example I want to debug this problem. I want to somehow install MY OWN COPY of stryker. What are the ways?
I think I can just: npm i -g /path/to/stryker. Will this work? Is this best practise?
Any other ways?

By the way: I started a conversation in your gitter. I get Stryker is not installed on stryker init right after installing stryker locally.

That shouldn't be happening. Does it still occur? If so, we should add a new issue.

I think I can just: npm i /path/to/stryker. Will this work? Is this best practise?

Install can be indeed be done with npm i file:/path/to/stryker. You should first build the ts files, but you know that by now 😉 . Since npm5 npm i file:... creates a symlink instead of installing it locally like you might expect. To get the local installation behavior i suggest to use (install-local)[https://npmjs.org/package/install-local].

Running stryker init creates a temp folder that is not removed:

The init command should not create a .stryker-tmp folder at all. The reason that it does is because the implementation of StrykerTempFolder. Whenever that file is loaded, it immediately creates the folder. This is a very bad practice! Importing the file has a side effect (creation of the folder). This is counter intuitive.
The preferred solution here is to convert all functions in that file to a class. The constructor should than except a string containing the name of the folder to be created (with '.stryker-tmp' as the default). In the constructor, it can create the temp folder on disk. At the beginning of the mutation test method in Stryker, it should create a new instance of the class. All classes using StrykerTempFolder should now use the instance created by Stryker.

....Does it still occur?...

I hope I will be able to check this at the evening.
Installation. Will npm link work? Will it be a problem to debug locally installed stryker? As I remember I had added node debug configuration to webstorm and it worked (I debugged grunt-* packages this way) and my own packages.

Whenever that file is loaded, it immediately creates the folder

Exactly. I already realized that too.

Will npm link work?

Yes it will.

Am I right that all classes wrapped with namespaces are implementations of singleton pattern? Singleton pattern is what I need there. Am I right? ConfigWriterFactory is one of the examples.
So first one who is working with the class will create an instance and next will use already created instance. I will work with that instance through instance() method, right?

So the plan is:

convert StrykerTempFolder to a singleton
refactor usages of current implementation (in most cases simply by adding .instance())
refactor units
look like thats all

Sounds good. The factory pattern with namespace is pretty complex. You can also do it this way:
class TempDirectory {
  private constructor(){}
  private static _instance: TempDirectory;
  static instance() {
     if(!this._instance){
        this._instance = new TempDirectory();
     }
     return this._instance;
  }
}

Not sure why we didn't use that with our factories.

I found that too when I researched about how to implement singletons. I think this is more new approach. Yes it is also much smaller and prettier.
I wanted to implement namespace approach since you used it in stryker. If its ok that I will do same another way - its ok. I will.

What about unit tests?
Here is the code:
sandbox.stub(StrykerTempFolder, 'createRandomFolder').returns(workingFolder);
sandbox.stub(StrykerTempFolder, 'copyFile').returns(Promise.resolve({}));
sandbox.stub(StrykerTempFolder, 'writeFile').returns(Promise.resolve({}));

Now I can not use StrykerTempFolder so. I need something like this:
...
sandbox.stub(StrykerTempFolder.instance(), 'createRandomFolder').returns(workingFolder);
...

Or, since there are a lot of usages of that instance I think it is better to declare const let one time, define it in beforeEach and use them in the tests.
What do you think?

I started. Here is my first results. I converted bundle of functions to the class. Here is commit: https://github.com/sharikovvladislav/stryker/commit/4628f0fb0306f8eb800948eed7426f8f6be11bfb
Or should I open a PR and we will discuss it in it?
Plan:

name. is current name is ok?
refactor codebase
refactor current units
perhaps units (do we need units for StrykerTempFolder? there are no ones)

name. is current name is ok?

Maybe we can call it TempFolder as it makes more sense (we're already in the Stryker repository after all).
On stubbing of StrykerTempFolder.instance(), you can use the mock function as done in the Stryker unit test: https://github.com/stryker-mutator/stryker/blob/master/packages/stryker/test/unit/StrykerSpec.ts#L55
It should look something like this:
import TempFolder from '../../src/utils/TempFolder';
import { mock, Mock } from '../helpers/producers';
import * as sinon from 'sinon';

describe('TempFolder', () => {
  let tempFolderMock: Mock<TempFolder>;
  let sandbox: Sinon.SinonSandbox;
  beforeEach(() => {
    tempFolderMock = mock(TempFolder);
    sandbox = sinon.sandbox.create();
    sandbox.stub(TempFolder, 'instance').returns(tempFolderMock);
    // now you are able to configure return values for tempFolderMock. For example:
    tempFolderMock.writeFile.resolves();
  });
  afterEach(() => sandbox.restore());
});

A Work in Progress PR is indeed easier i think.

I started a PR.

Did you check this? I think this can be closed now.

I just released it and it works like a charm! Thanks for fixing it!


Implement all Tendermint RPC methods

Following Tendermint RPC APIs are not implemented:

[x] #244
[x] Health
[x] NetInfo
[x] #233
[x] Genesis
[x] #232
[x] Block
[x] #234
[x] #235
[x] #236
[x] CheckTx
[x] #238
[x] #239
[x] #237
[x] #242
[x] #243
[x] #290
[x] UnconfirmedTxs
[x] #240
[x] BroadcastTxCommit
[x] BroadcastTxSync
[x] BroadcastTxAsync

#287 resolved last missing method.


Google reports errors in JS console

TypeError: Cannot set properties of null (setting 'textContent') at update (https://germanivanov0719.github.io/grades/version.js:29:49) at initFunction (https://germanivanov0719.github.io/grades/version.js:66:5)

fixed probably


Add option to lock waveform widget dual marker position relative to most recent streaming data.

Joulescope model
All
UI version
1.0.22
Your idea
With the Joulescope UI 0.10.x and earlier, the dual markers remained in a fixed location on the screen.  As the waveform scrolled by, the dual markers would display statistics from whatever part of the waveform was within that region.
We received lots of complaints about this behavior as it makes it difficult to analyze waveform features without pausing live streaming data.  For host computers with lots of RAM, customers wanted to keep running without losing part of the waveform.  This behavior was a problem.
With Joulescope UI 1.0, we update the UI to keep the dual markers fixed relative to the waveform.  As the waveform scrolls by, so do the markers until they eventually scroll offscreen.  This addresses the very common complaint in 0.10.x.
However, it turns out that some Joulescope UI users came to rely on this feature.  If you know you expected waveform, you can strategically add dual markers.  With appropriate spacing, you can simultaneously measure sleep (dual markers), active (same or second set of dual markers), and average (right-hand statistics).  The Joulescope UI 1.0 removes this ability.
The Joulescope UI should add a feature to have "floating" dual markers that behave like the 0.10.x and earlier markers.
I can't help but laugh.  The customer reporting this issue included this XKCD.
Does your idea concern a specific OS?
All

Added to 1.0.27


added verifyPhoneNumber (for Android), alwaysShowNotification, replacePrevNotifications

last week firebase added phoneAuth to the android sdk and since the JavaScript implementation doesn't work for cordova (without nasty workarounds) I added it.
However, I've never written a cordova plugin (or something in JAVA) before, so maybe you want to have a look ;)
Works fine on Android 7.1.2.

last commit adds
setAlwaysShowNotification (Android only?)
Shows notification when the app is in foreground.
Notifications behaves as if app is in background. That means the notification data is received in the JavaScript callback when the users taps the notification.
window.FirebasePlugin.setAlwaysShowNotification(true/false)

setReplacePrevNotifications (Android only?)
Previous notifications are replaced by new ones. Data of old notifications is lost.
window.FirebasePlugin.setReplacePrevNotifications(true/false)

heya @apptum  -  did you fork this?  Walk me through installing this please.   setAlwaysShowNotification - is it still for Android only?  What is the behavior on iOS?  I have yet to figure out how to process a payload on an inbound notification if the app is already open...if its closed or in the background then no problem, but opened in foreground I can't get find a way to see the payload.  I would ultiamtely prefer your new method, but at a minimum if I can get the payload while app is open then I could at least do some kind of custom pop up - have you figured this one out yet?

Does it relate to https://github.com/arnesson/cordova-plugin-firebase/pull/325?

@apptum - this fork is crashing my app.  I installed this fork and added:
window.FirebasePlugin.setAlwaysShowNotification(true) ;
I am sending test messages from FCM console to my device and everytime a message arrives it crashes my app - console shows now messages or errors.  With the app open and in the foreground, the app crashes/closes and a message appears on the Android: "Unfortunately, APPNAME has stopped".
I did notice it created a "cordova-plugin-firebase" folder in the appName/platforms/android folder.   But I can't find any other crash files or logs.

As well when you try
FirebasePlugin.verifyPhoneNumber(phone, 60, function (credential)

I always get error: unknown error verifying number

Since @apptum got it working on Android 7.1, my guess is that this fork is not backward compatible with Android 6x...or probably even 5x.

@sajTempler its probably an error in your app configuration in the firebase. did you added the app SHA1 fingerprint? take a look in #418

@brunosoaresds yes I solved this couple days ago, in deed this was SHA-1 concern. As well maven repository (as in your pull request). Thanks

closing this since a newer PR has been merged


Update sha for corefx to pick up change to generate version by default

The bootstrap script fails with a source-build dotnet because it can't find the version information that its looking for.  This picks up the change from corefx that generates version information by default.
Fixes #651

Could you point out the commit(s) this is needed for (for reference)? Just https://github.com/dotnet/corefx/pull/31161?

@dotnet-bot test OSX10.12 Release
java.nio.channels.ClosedChannelException

Same machine built both of the last failures, dnfed28op000001. (In the Building remotely on... line near the top.) Filing a core-eng issue.

@dotnet-bot test Fedora28 Release
Old machine was deleted, new machine is up with a bit more space. (Although we aren't 100% sure space is the only issue.)

Further issues with the machine are tracked by https://github.com/dotnet/core-eng/issues/4159 which is being fixed.

@dotnet-bot test Fedora28 Release


Cadence breaking changes

There have been a handful of Cadence server releases since the last time the images was updated.  In the branch cadence, I have updated the Cadence server, Cassandra, Golang, Cadence go-client, and Cadence web UI to the most recent versions that are compatible with one another.  There are some breaking changes that need to be fixed on the .NET side.
The breaking changes that I can identify so far are:

go.uber.org/cadence/.gen/go/shared.DomainConfiguration has removed the fields ArchivalBucketName and ArchivalStatus and have added HistoryArchivalURI, HistoryArchivalStatus, VisibilityArchivalStatus, and VisibilityArchivalURI
go.uber.org/cadence/workflow has removed the field ChildPolicy and replaced it with ParentClosePolicy
go.uber.org/cadence/workflow has removed the type ChildWorklfowPolicy
go.uber.org/cadence/client has added the type ParentClosePolicy
There is now a limit on go.uber.org/cadence/.gen/go/shared/RegisterDomainRequest.WorkflowExecutionRetentionPeriodInDays.  We were originally setting this in ConnectRequest to create the default domain if specified.  The default value that we set for this was int32(365).  The new limit is int32(30)

There are currently 2 tests failing in Test.Neon.Cadence:

TestCadence.Test_Messages.Test_WorkflowDescribeExecutionRequest
TestCadence.Test_Messages.Test_WorkflowExecuteChildRequest

These failures are related to the breaking changes mentioned about.

Adding @marcusbooyah because we'll probably need to upgrade Cadence/Cassandra in production to support this.

Changes in progress:

I modified cadence-proxy to use retentionDays=7 when it creates a default domain on connect.  This is consistent with the default of 7 days when a domain explicitly created by the user.

Merged for v2.0.0


Mobile footer breaks if there is only one column set

If the MX footer is set like this (which is the desired set up): https://thor.dosomething.org/mx/admin/appearance/settings/paraneue_dosomething
Then the footer appears like this on mobile:

cc: @julielorch

Oof! Yeah, that's no good.

Talked with @julielorch and we're going to fix this by spreading things between the three columns. :hammer:

@DFurnes if we are solving this with a content fix, should we close this issue?

Is the fix tracked in Trello or somewhere else? Or do you have your own todo list @julielorch?

Closing this out because we're going with a content solution of spreading the footer out across the three columns.


Not Headless and more examples

As a performance, I do not have experience w/ Puppeteer. But I would like to heavily use to troubleshoot the client-side performance using Memlab.

How do we run with non-headless mode? If I try with headless: false (from Puppeteer doc), it is not working.
It would be great, if we need more examples on the automation front.

Thanks.

@QAInsights For MemLab CLI, use --headful to run memlab in headful mode, for example:
memlab run --scenario <scenario file> --headful

Please check out the documentation for more examples:
https://facebookincubator.github.io/memlab/docs/intro

Thanks @JacksonGL Yes, I already went throught the examples. I am looking for automation scripts e.g. login, extracting token/sessions etc.

@QAInsights The automation part is done by invoking methods in page object from Puppeteer, there are plenty of examples online for automation with Puppeteer.
Puppeteer documentation: https://pptr.dev/


Warning message when removing targets: Special characters appearing

To reproduce the issue, open up the Targets tab for an indicator that already has targets set up. Click the "Remove all targets" link next to the grayed-out "Create targets" button. You'll see the following warning message. Note the \n\n characters before the final sentence in the paragraph.

This appears to be a duplicate of #657 that is fixed.


Properly format file path on when dragging and dropping a tab into the integrated terminal in Windows

This is a proof-of-concept illustrating a possible fix for #29926.
It uses the wmic process where parentProcessId=... get ... command to query processes spawned by the shell and matches the process executables with a set of known shells (bash.exe, cmd.exe, and powershell.exe). In the case that one of these spawned processes is a shell, its child processes are recursively examined until either there are no more child processes (nothing's running in the shell) or there are only non-shell child processes (a command is running).
After traversing these nested shells, the path of the last reached shell is used to pick a formatter for the path (either putting it in C:\..., /mnt/c/ or /c/ format).
A gif of using different shells is below.

The way this is implemented currently has a few disadvantages though:

For every nested shell that is traversed a new wmic command must be spawned. When the number of nested shells increases, it can take some time before the path is pasted. This could be solved by caching the current shell, but I wasn't sure how to go about this.

This fails to work correctly in cases that a shell is spawned in the background (i.e. start cmd.exe) as there are no checks to whether the shell being traversed is in the background or foreground.

I'm not going to be able to get back to this for a while soon, so feel free to play around with / heavily edit this.

@rianadon,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

I'm not going to be able to get back to this for a week and a half and I
should have thought of this earlier, but the delay could probably be
reduced by running the wmic command only once, and without the where
clause, then parsing the output and storing the executables and process
pids by parent pid. Then this could be used for lookup in getChildProcesses
instead of launching a new command.
There would still be a 0.5 to 1s delay, but hopefully at least it wouldn't
get to the length it gets to the end of the gif with lots of nested
terminals. Polling would definitely help.

@rianadon no problem, get to it when you can.
I believe @Lixire may be looking into https://github.com/Microsoft/vscode/issues/30152 (and https://github.com/Microsoft/vscode/issues/29879) in the July iteration so feel free to just simplify this PR to look at the initial shell executable. After #30152 we should hopefully have reasonably up to date knowledge of the shell at any time.

Now that @Lixire has adapted this to change the terminal title and has also made what I had much cleaner and efficient (to the point where I don't think running the command once to get all processes would make a difference), is all that's left using WindowsShellHelper when the tab is dropped onto the terminal? Or is there anything else that needs to get done?
And also since this has already been merged via #30732 I think I would need to create another PR, right?

@rianadon you would need to modify WIndowsShellHelper to keep track of the actual shell. Right now it sets the title of the TerminalInstance to the executable under the last recognized shell, for example the following process tree:

cmd.exe (last shell)

git.exe (set terminal instance title to git)

git.exe

less.exe

You would also need to differentiate git bash and wsl bash which isn't being done currently.
So I would expect something like:

WindowsShellHelper.getShellName should be renamed to something like WindowsShellHelper.updateProgramName
Track the shell name inside WindowsShellHelper, this could be pulled into TerminalInstance but since we don't have this yet for Linux/macOS let's keep it into WindowsShellHelper
A new WindowsShellHelper.getShellName that actually just returned the shell that was acquired from the last updateProgramName

FYI there's also this change which will improve the reliability of the title on Windows for commands that exit by themselves, like git status, npm install, etc. https://github.com/Microsoft/vscode/pull/31155.

@Tyriar Thanks for the advice. I've revised this in #31234.


[Typography] Add system props

Another iteration on #24485

I guess we can migrate the usages of the component in the codebase in a different iteration? (Both for Box and Typography)

Definitelly, didn't want to do all those changes in just one PR :)

I guess we can migrate the usages of the component in the codebase in a different iteration? (Both for Box and Typography)

Definitelly, didn't want to do all those changes in just one PR :)

I don't understand why this started failing now - https://app.circleci.com/pipelines/github/mui-org/material-ui/36615/workflows/0ea8cae7-7ee2-4350-881b-06df6d568b69/jobs/217634 it's related to the typings of the LinkProps. As far as I understand the OverrideProps should add the component prop.

I don't understand why this started failing now - https://app.circleci.com/pipelines/github/mui-org/material-ui/36615/workflows/0ea8cae7-7ee2-4350-881b-06df6d568b69/jobs/217634 it's related to the typings of the LinkProps. As far as I understand the OverrideProps should add the component prop.

I don't understand why this started failing now - https://app.circleci.com/pipelines/github/mui-org/material-ui/36615/workflows/0ea8cae7-7ee2-4350-881b-06df6d568b69/jobs/217634 it's related to the typings of the LinkProps. As far as I understand the OverrideProps should add the component prop.

@oliviertassinari @eps1lon I could use some help here. I spend 40 mins trying to understand what is happening but with no success 😞

I don't understand why this started failing now - https://app.circleci.com/pipelines/github/mui-org/material-ui/36615/workflows/0ea8cae7-7ee2-4350-881b-06df6d568b69/jobs/217634 it's related to the typings of the LinkProps. As far as I understand the OverrideProps should add the component prop.

@oliviertassinari @eps1lon I could use some help here. I spend 40 mins trying to understand what is happening but with no success 😞

Looking at it right now

Looking at it right now

I can't find any mention of what the plan for the color prop is. You're saying your just adding the system props but you didn't explain what happens with the props that already exist on Typography and are implemented differently in the system props. For reviewability please use PR descriptions to summarize your work.
Typography is extended by a lot of other components so just changing how color works isn't trivial. At least not for me and considering CI is failing it's not trivial for you either.

I can't find any mention of what the plan for the color prop is. You're saying your just adding the system props but you didn't explain what happens with the props that already exist on Typography and are implemented differently in the system props. For reviewability please use PR descriptions to summarize your work.
Typography is extended by a lot of other components so just changing how color works isn't trivial. At least not for me and considering CI is failing it's not trivial for you either.

I can't find any mention of what the plan for the color prop is. You're saying your just adding the system props but you didn't explain what happens with the props that already exist on Typography and are implemented differently in the system props. For reviewability please use PR descriptions to summarize your work.
Typography is extended by a lot of other components so just changing how color works isn't trivial. At least not for me and considering CI is failing it's not trivial for you either.

I should have explained the breaking changes for the classes in the PR description, my bad will do it. Regarding the color prop, with https://github.com/mui-org/material-ui/pull/24496/files#diff-e28433586a43ba335e74439005786b53bce7291ed0e5062a5a13d8254775c9ebR79 I've added backward compatibility for the colors we had before. The plan is to keep this working as it was before and in the next major version v5.x deprecate the support for these custom colors and remove the transformation in v6, so basically this change regarding the color prop should not be breaking at this moment (typings should be just relaxed in terms of changes).

I can't find any mention of what the plan for the color prop is. You're saying your just adding the system props but you didn't explain what happens with the props that already exist on Typography and are implemented differently in the system props. For reviewability please use PR descriptions to summarize your work.
Typography is extended by a lot of other components so just changing how color works isn't trivial. At least not for me and considering CI is failing it's not trivial for you either.

I should have explained the breaking changes for the classes in the PR description, my bad will do it. Regarding the color prop, with https://github.com/mui-org/material-ui/pull/24496/files#diff-e28433586a43ba335e74439005786b53bce7291ed0e5062a5a13d8254775c9ebR79 I've added backward compatibility for the colors we had before. The plan is to keep this working as it was before and in the next major version v5.x deprecate the support for these custom colors and remove the transformation in v6, so basically this change regarding the color prop should not be breaking at this moment (typings should be just relaxed in terms of changes).

The autocomplete for the previously known color values is lost at the moment. Are unknown values for color currently accepted?

The autocomplete for the previously known color values is lost at the moment. Are unknown values for color currently accepted?

The autocomplete for the previously known color values is lost at the moment. Are unknown values for color currently accepted? That creates two problems:

no static safety against wrong values
worse migration story in the future when we change these values

The autocomplete now shows only the CSS valid color values, for the theme's values the typing is currently string. Mainly, this is done because the theme can have custom colors.. Maybe we can improve the color type to pick up values from theme and use the new Template Literal Types, not sure how the perf of it will be tough...
The other option is, for the Typography component we can have a custom color type that will list all prev values + string. Do you think this will be a better option? That's at least what we have done so far with the Button and Badge components.

The autocomplete for the previously known color values is lost at the moment. Are unknown values for color currently accepted? That creates two problems:

no static safety against wrong values
worse migration story in the future when we change these values

The autocomplete now shows only the CSS valid color values, for the theme's values the typing is currently string. Mainly, this is done because the theme can have custom colors.. Maybe we can improve the color type to pick up values from theme and use the new Template Literal Types, not sure how the perf of it will be tough...
The other option is, for the Typography component we can have a custom color type that will list all prev values + string. Do you think this will be a better option? That's at least what we have done so far with the Button and Badge components.

Are unknown values for color currently accepted?

@eps1lon I would hope so, shouldn't behave like Box sx={{ color?

Are unknown values for color currently accepted?

@eps1lon I would hope so, shouldn't behave like Box sx={{ color?

The autocomplete now shows only the CSS valid color values, for the theme's values the typing is currently string. Mainly, this is done because the theme can have custom colors..

I understand. Could you respond to my part about the currently known values.

I would expect unknown values for color to be accepted.

Why?

Maybe we can improve the color type to pick up values from theme and use the new Template Literal Types, not sure how the perf of it will be tough...

Never worry about perf before the implementation. You'll almost never be able to judge the actual perf impact without meassuring.
Is this another PR that was discussed internally? Would be nice to mention this before so potential reviewers don't waste time. Until recently reviewability of PRs was the default but these PRs are more and more unclear due to lack of description.

The autocomplete now shows only the CSS valid color values, for the theme's values the typing is currently string. Mainly, this is done because the theme can have custom colors..

I understand. Could you respond to my part about the currently known values.

I would expect unknown values for color to be accepted.

Why?

Maybe we can improve the color type to pick up values from theme and use the new Template Literal Types, not sure how the perf of it will be tough...

Never worry about perf before the implementation. You'll almost never be able to judge the actual perf impact without meassuring.
Is this another PR that was discussed internally? Would be nice to mention this before so potential reviewers don't waste time. Until recently reviewability of PRs was the default but these PRs are more and more unclear due to lack of description.

We discussed on the last team meeting about adding the system props on the Box, Typography and Grid components. I am now trying and opening PRs for this. Up until know we didn’t know about what issues we will have (SystemProps not working, color types will be relaxed etc) I believe PR reviews are the right place for discussing it, which is what we are doing. Currently the typings for the color prop are identical as the sx’s color prop, which I believe should stand as they should be the same thing. I agree that this component is more problematic as it had the color prop, so let’s try to find the best solution and be constructive.

We discussed on the last team meeting about adding the system props on the Box, Typography and Grid components. I am now trying and opening PRs for this. Up until know we didn’t know about what issues we will have (SystemProps not working, color types will be relaxed etc) I believe PR reviews are the right place for discussing it, which is what we are doing. Currently the typings for the color prop are identical as the sx’s color prop, which I believe should stand as they should be the same thing. I agree that this component is more problematic as it had the color prop, so let’s try to find the best solution and be constructive.

I have a question what about Typography when it will be migrated to UnstyledTypography we will no have color property and we won't have sx property how I am supposed to style Typography component properly? If want to use my styling with scss and jss for example.

I have a question what about Typography when it will be migrated to UnstyledTypography we will no have color property and we won't have sx property how I am supposed to style Typography component properly? If want to use my styling with scss and jss for example.

@povilass The Typography is a CSS helper component. We didn't plan it move it to unstyled, but maybe there is a use case.

@povilass The Typography is a CSS helper component. We didn't plan it move it to unstyled, but maybe there is a use case.

I am looking for a framework solution and I want to have all material styled-components as unstyled. I want to just take this 1 library and use it with my core solution to style components nothing more. Now I have to think about how to implement helper components. Just a concern.

I am looking for a framework solution and I want to have all material styled-components as unstyled. I want to just take this 1 library and use it with my core solution to style components nothing more. Now I have to think about how to implement helper components. Just a concern.


Add indicators for lines that have been wrapped

I think a nice feature to add would be indicators if a line is wrapped.

Non-Wrapped
1       public static void main(String[] args) {
2              System.out.println("Wow. Wrapping text is so, so cool")
3       }

Wrapped
1       public static void main(String[] args) {
2              System.out.println("Wow. Wrapping text
        is so, so cool")
3       }

Wrapped Indicated 1st Way
1       public static void main(String[] args) {
2              System.out.println("Wow. Wrapping text
        󱞩 is so, so cool")
3       }

Wrapped Indicated 2nd Way
1       public static void main(String[] args) {
2              System.out.println("Wow. Wrapping text
               󱞩 is so, so cool")
3       }

I used this type of wrapping in Jupyter Notebook a while ago in school, and I found it to be the best way to recognize a wrapped line (Jupyter Notebook uses the 2nd way I proposed). If for some reason you can't see it, that icon is NerdFont md-arrow_right_bottom.

I think there could be options for indicator icon, color, and whether or not to put it in the first character spot on the new line where the wrapped line starts (like in my 1st example), or under the first character under the line that is being wrapped (like in my 2nd example) .

I'd totally make this, but I'm useless at Lua :)

Sorry for the code block. It was the only way I could get Github to recognize my weird formatting.

This is an interesting idea. I think to implement this would probably need NeoVim extmarks. It's not easy though, as they'll move around as the text is edited.
I'll leave this open here in case anyone wants to take this on, but right I'm not sure it's something I can spend any time on, it's probably non-trivial. I think this would ultimately be better as a built-in NeoVim feature.

Thanks for your interest, Andrew!
I definitely agree that it won't be trivial to implement, but I forked this repo and I'll give it my best shot with a buddy over the next week or so but by all means if someone wants to save me :wink:

Hi! Now that Nvim 0.10 is out and has the status column feature, could this feature be implemented in the next update?

There's no easy way to do this in the general sense; the vim.opt.statuscolumn option is typically used for a number of things, such as customizing line numbers, diagnostic marks, etc. Everyone uses it differently (or will), so an implementation in wrapping.nvim by itself wouldn't really make sense and would probably out of the scope of this plugin. It would probably make more sense to request this as a feature in a status column plugin like https://github.com/luukvbaal/statuscol.nvim, since there is nothing specific to wrapping.nvim about the lines that are wrapped; it's NeoVim itself that does the wrapping, not this plugin.
For now, I'm going to close this issue. Thanks for your thoughts and input.


[SmartContract] Add Migration Code

This revision implements follow

Add a Migration code
Modify MintShaderToken.sol
Add jsonfiles for tracking address of contracts

You can test the code with two terminal commands
npx hardhat node
npx hardhat run scripts/script.js --network localhost

LGTM 😊


Upgrade GitHub Actions and lint with codespell and ruff

Ruff supports over 500 lint rules and can be used to replace Flake8 (plus dozens of plugins), isort, pydocstyle, yesqa, eradicate, pyupgrade, and autoflake, all while executing (in Rust) tens or hundreds of times faster than any individual tool.
ruff --format=github rapidly provides intuitive GitHub Annotations to contributors.

Typos discovered by codespell
% codespell
./CONTRIBUTING.md:6: wether ==> weather, whether
./CONTRIBUTING.md:12: wether ==> weather, whether
./CHANGELOG.md:29: simplfy ==> simplify
./doc/source/pymatting.bib:188: Mor ==> More
./doc/source/intro.rst:33: developement ==> development
./pymatting/laplacian/lkm_laplacian.py:25: returnes ==> returns
./pymatting/solver/cg.py:34: Wether ==> Weather, Whether
./pymatting/util/util.py:199: wether ==> weather, whether
./pymatting/util/util.py:762: Additionaly ==> Additionally
./benchmarks/solve_petsc.c:92: inizialize ==> initialize

Ruff issues ignored
60	F405	[ ] `apply_to_channels` may be undefined, or defined from star imports: `pymatting`
49	F401	[*] `hashlib` imported but unused
31	E501	[ ] Line too long (130 > 127 characters)
14	F403	[ ] `from pymatting import *` used; unable to detect undefined names
 2	E731	[*] Do not assign a `lambda` expression, use a `def`
 2	E741	[ ] Ambiguous variable name: `I`
 1	F811	[ ] Redefinition of unused `cg` from line 32

@99991 The tests that this pull request modifies do not require those image.

Thank you for your contribution! :tada:


Added TomerFi profile

Hey
Cool project, thank you for sharing.
I added myself to the pepole folder.
Have a great day.

Good to have you here :)