Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request for Array method #114

Closed
lefouvert opened this issue Jan 19, 2024 · 18 comments
Closed

Request for Array method #114

lefouvert opened this issue Jan 19, 2024 · 18 comments
Labels
enhancement New feature or request probably fixed Probably fixed, but should be confirmed stale Issue has not seen activity for 60 days

Comments

@lefouvert
Copy link

lefouvert commented Jan 19, 2024

Hi, it would be wonderfull if Array type implement those two methods :

  • Sort
  • Reduce

(or OrderBy and Aggregate as said in .net framework)

And it would be nice we could edit collection, with some kind of .Add .Remove.

Kindly,
LeF.

Edit : typo

@untoldwind untoldwind added the enhancement New feature or request label Jan 20, 2024
untoldwind added a commit that referenced this issue Jan 20, 2024
untoldwind added a commit that referenced this issue Jan 20, 2024
@untoldwind
Copy link
Owner

The sort is easy. With the reduce I encountered some issues with the type-inference:
Something like this works:

    let int_arr = [ 17, 11, 3, 7, 5, 13 ]
    
    let sum = int_arr.reduce(0, fn(sum : int, item: int) -> sum + item)

but when removing the type-hints from the inner function (i.e. just int_arr.reduce(0, fn(sum, item) -> sum + item)) the compiler is currently not able to deduce the return type. There is something of a hen-egg problem with the current implementation.

As for the add/remove: I was very careful to keep the array immutable (mostly because I had some very bad experiences with javascript in this regard 😮‍💨 ).
The array does support the + and += operators though.

So something like this works:

let int_arr : int[] = []
for(i in 0...4) {
  int_arr += i
  int_arr = int_arr + i // same as above, note: int_arr + i creates a new array
}

I also added a slice method, so that removal of an element could be done like this:

int_arr.slice(0, to_remove) + int_arr.slice(to_remove + 1)

If you really need a mutable array/list/queue I would rather like to add specific type for that

@lefouvert
Copy link
Author

If you really need a mutable array/list/queue I would rather like to add specific type for that

As I come from old fashionned procedurals languages (C, ASM 68k, Cobol), even if I have fun with more recent approach, I still struggle to apprehend all subtilities of those.

I think you are right, an array should be unmutable.

Since it was the only collection, I was thinking about it only as collection, but if it's still in language phylosophy, an other kind of collection, as a list, or a set could be usefull, and it would preserve the unmutability of the array. (Unless unmutability is expected for all kind of collection ^^)

Keep in mind that although I knew the existence of functionnal language, Kontrol is my first time encounter - I kinda like it :) - , so my request could be irrelevant : maybe it's on me to learn to work differently.

+= slice

Thanks for the tips !

With the reduce I encountered some issues with the type-inference

When I first used a reduce, that's exactly what I asked myself: "How the hell do they know which type goes where?" ". I'm kind of glad I'm not alone.

The sort is easy

As expected :)

very bad experiences with javascript

Imagine a black cat, all bristly fur, hissing while threatening with his claws. It's me when I code in javascript. The only times I approach this 'thing' is when I only have a highly restricted authority computer which don't allow me to install any sdk. I feel your misgivings.

@untoldwind untoldwind added the probably fixed Probably fixed, but should be confirmed label Jan 22, 2024
@untoldwind
Copy link
Owner

This should now be part of 0.5.2.4

    const sum = int_arr.reduce(0, fn(sum, item) -> sum + item)

also works, though the type-inference certainly needs more testing

@lefouvert
Copy link
Author

lefouvert commented Jan 23, 2024

version 0.5.2.4 (Ckan)

Need a dumb monkey doing dumb things to nice tools to see if they are dumbproof? Here I am!

gitreport::reduce.to2

use { Vessel } from ksp::vessel

fn sumint() -> int = {
    const arr_int = [1, 2, 3, 4, 5, 6]
    const sum = arr_int.reduce(0, fn(sum, item) -> sum + item)
    return sum
}
// Rebooted in 00:00:02.9630349
// No errors

// Doc refer to vessel.part as an array : 'parts | ksp::vessel::Part[] | R/O | Get a list of all vessel parts.'
fn fuel_flow(vessel: Vessel) -> float = {
    return vessel.parts
        .filter(fn(p) -> p.engine_module.defined)
        .map(fn(p) -> p.engine_module.value.max_fuel_flow)
        .reduce(0.0, fn(sum, f) -> sum += f)
}
// Rebooted in 00:00:02.5323374
// ERROR: [gitreport\reduce.to2(17, 36)] NoSuchVariable
// Local variable 'sum' is read-only (const)


// Also broke, but less intriging since I use a Range
fn factorial(n: int) -> int = {
    return (1...n)
        .reduce(1, fn(f, x) -> f *= x)
}
// Rebooted in 00:00:02.6255638
// ERROR: [gitreport\reduce.to2(27, 18)] NoSuchMethod
// Type 'Range' does not have a method or field 'reduce'
// ERROR: [gitreport\reduce.to2(26, 1)] IncompatibleTypes
// Function 'factorial' returns Unit but should return int


// Also broke, even if I trick the compiler converting the Range into an Array
fn factorial(n: int) -> int = {
    return (1...n)
        .map(fn(x) -> x)
        .reduce(1, fn(f, x) -> f *= x)
}
// Rebooted in 00:00:02.6332422
// ERROR: [gitreport\reduce.to2(42, 32)] NoSuchVariable
// Local variable 'f' is read-only (const)

Edit : misspelling in comments
Edit : code readability

@untoldwind
Copy link
Owner

untoldwind commented Jan 23, 2024

The reduce method for ranges will be added with the next patch.
The Local variable 'f' is read-only error though is kind of by design, i.e. all function parameters are treated as const.

In your example: If you just replace fn(f, x) -> f *= x with fn(f, x) -> f * x it should work.

After all reduce is not a loop: [item0, item1, item2].reduce(initial, reducer) is supposed to be equivalent to:
reducer(reducer(reducer(initial, item0), item1), item2)))

@lefouvert
Copy link
Author

lefouvert commented Jan 23, 2024

Han..!
I now see where I diverge from your exemple.
Maybe it's tiredness that's crumbled me.
I'll take a nap before have fun with those .reduce :)

Thank you !

@lefouvert
Copy link
Author

Hello again :)
I've just seen that .sort doesn't take any parameters, so I assume sorting only works on arrays whose type is compatible with standard comparison operators.
Is it possible to do this with an (optional?) parameter that defines a comparison subject for complex structures (like a Record, for example, or, in my test case, the activation_stage of a part :))

@untoldwind
Copy link
Owner

I had some problems with an optional function parameter, so I added .sort_by and .sort_with instead.

Example:

pub struct TestStruct(i: int) {
    field1: int = i
}

const struct_arr : TestStruct[] = [
        TestStruct(1),
        TestStruct(-6),
        TestStruct(2),
        TestStruct(10),
        TestStruct(-4),
    ]

const sort_asc = struct.arr.sort_by(fn(t) -> t.field1)
const sort_desc = struct_arr.sort_by(fn(a, b) -> if(a.field1 > b.field1) -1 else if(a.field1 == a.field2) 0 else 1)

I.e. `.sort_by` is mapping the element to something that is sortable (like int/string) and `.sort_with` is using an explicit comparator.

@lefouvert
Copy link
Author

lefouvert commented Jan 24, 2024

version 0.5.2.5 (Ckan)

sort_* are perfect. Thank you.


Since it's also about Arrays, I have a question.
In the doc, I could read about tuples

use { floor } from core::math
fn floor_remain(a : float) -> (int, float) = (floor(a).to_int, a - floor(a))

In assignments tuples can be deconstructed:

 let (b, c) = floor_remain(12.34)

will define an integer variable b with value 12 and a float variable c with value 0.34

I feel (maybe wrongly) than .filter and co. make an assigment in the fn(something) -> declaration.
So I tried this :
gitreport::arrtupledeconstruct.to2

fn foo() -> (int, string)[] = {
    const foo = [(0, "zero"), (1, "one"), (2, "two"), (3, "three")]
    return foo
        .filter(fn((number, letter)) -> number == 2)
}

fn bar() -> (number: int, letter: string)[] = {
    const bar = [(number: 0, letter: "zero"), (number: 1, letter: "one"), (number: 2, letter: "two"), (number: 3, letter: "three")]
    return bar
        .filter(fn(rec) -> rec.number == 2)
}

and I get this error from foo function:

Rebooted in 00:00:02.4392074
ERROR: [gitreport\arrtupledeconstruct.to2(5, 16)] Parsing
gitreport\arrtupledeconstruct.to2(5, 16): Expected <whitespace>

Is that expected ?

Anyway, as you can see, bar function, even if it doesn't deconstruct the record, work perfectly and is a satisfying workaround. It's just to u know, if it wasn't expected.

Edit : typo

@untoldwind
Copy link
Owner

Sorry, overlooked this comment.
So far I have not implement deconstructing in function/lambda parameters yet, so the offending part is: fn((number, letter)) -> number == 2

Instead you could do it like this:

    return foo
        .filter(fn(tuple) -> tuple._1 == 2)

@lefouvert
Copy link
Author

It happen. And I'm maybe a bit talkative, too ^^
I had uderstood the unimplmented deconstructing in fn/lambda, but i'm happy to learn than tuple items can be accessed this way !

Copy link

github-actions bot commented Apr 1, 2024

This issue is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale Issue has not seen activity for 60 days label Apr 1, 2024
@lefouvert
Copy link
Author

lefouvert commented Apr 13, 2024

Hi !

It seems VSC extention have some trouble to understand the new property array.is_empty for unusual cases :

sync fn group_statics_as_rotaries(staticSolarPanels: Part[]) -> Part[][] = {
    if(staticSolarPanels.length < 2)
        return []

    (0..staticSolarPanels.length)
        .flat_map(fn(i) -> ((i + 1)..staticSolarPanels.length).map(fn(j) -> normal_for_partpair(staticSolarPanels[i], staticSolarPanels[j])))
        .reduce(<(parts: Part[], normalId: GlobalVector)>[], group_by_normal)
        .map(fn(r) -> r.parts)
        .filter(exclude_asymetric_enlightenment)
        .sort_by(fn(a) -> a.length)
        .reverse()
        // .reduce(<Part[]>[], deduplicate) // TODO
        .reduce(<Part[]>[], fn(clean, partArr) -> {
            if(clean.is_empty)
                return clean + partArr
            return clean + partArr.filter(fn(part) -> !clean.flat_map(fn(a) -> a).exists(fn(p) -> p.position == part.position)) // TODO prefer a part.id than a part.position when it will be available
        }) // deduplicate
        .filter(fn(a) -> a.length > Array.Empty)
}

leads to

Undefined field is_empty for type Part[][]

on if(clean.is_empty), for example.

Note : Same struggles happen with Records array as seen on this alert (or is it because it's somewhat matricial with an array in an other ?):

Undefined field is_empty for type (parts : Part[], normalId : GlobalVector)[]

And since I find .is_empty a really nice adding (You have seen my Array.Empty, didn't you ? ^^), could string benefit from the same features ?
Cherry on the cake would be have a empty property on string which could allow us to use some string.empty as equivalent of "". (should be linked with issue #123)

@lefouvert
Copy link
Author

Still on arrays, but other anomaly, about += operator :
I have this :

pub struct Logger() {
    blackboxes: (vesselUID: string, entries: LogEntry[])[] = []
}

impl Logger {
// some other methods...
    fn new_entry(self, entry: LogEntry) -> Unit = {
        self.blackboxes = self.blackboxes + (vesselUID: entry.vessel.id, entries: [entry])
    }
}

Everything goes fine.

BUT !
If I dare to write

    fn new_entry(self, entry: LogEntry) -> Unit = {
        self.blackboxes += (vesselUID: entry.vessel.id, entries: [entry])
    }

I got this error :

Rebooted in 00:00:03.7620438
ERROR: [ui\logger.to2(53, 9)] IncompatibleTypes
Type 'Logger' field 'blackboxes' is of type (entries : ui::logger::LogEntry[], vesselUID : string)[] but is assigned to (entries : LogEntry[], vesselUID : string)

@github-actions github-actions bot removed the stale Issue has not seen activity for 60 days label Apr 14, 2024
@untoldwind
Copy link
Owner

Arrays are somewhat special and mostly hardcoded ein the vscode extension, I just forgot to add the is_empty there. This version should fix it: https://github.com/untoldwind/KontrolSystem2/releases/download/v0.5.8.3/to2-syntax-0.0.50.vsix

The += is an issue on the field assignment (it works for variable assignments). Should be fixed in the next release

@untoldwind
Copy link
Owner

Should work in 0.5.8.4

Copy link

This issue is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale Issue has not seen activity for 60 days label Jun 15, 2024
Copy link

This issue was closed because it has been inactive for 14 days since being marked as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request probably fixed Probably fixed, but should be confirmed stale Issue has not seen activity for 60 days
Projects
None yet
Development

No branches or pull requests

2 participants